Skip to content

[🧱 MCP Server Tool] Project Structure Cleanup#2699

Merged
bendvc merged 8 commits intofeature/pwa-developer-agent-onlyfrom
bendvc/clean-up-project-structure
Jul 2, 2025
Merged

[🧱 MCP Server Tool] Project Structure Cleanup#2699
bendvc merged 8 commits intofeature/pwa-developer-agent-onlyfrom
bendvc/clean-up-project-structure

Conversation

@bendvc
Copy link
Contributor

@bendvc bendvc commented Jun 30, 2025

Description

🧹 This PR gives our mcp server project a little spring cleaning (in June, because who follows rules anymore?). The goal was to make things less "Where the heck is that file?" and more "Ah, there it is."

This PR:

  • Make the project use commonr patters from other PWA projects.
  • Add build step so we can use imports without having to be a module type project (this didn't work well with out jest integration)
  • No need to use .js terminations (this follows the same pattern as the rest of the mono repo). We could change our project to be a esm in the future but that might take more time.

No functionality changes. Just vibes and structure. 🏗️

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Changes

  • Removed unused or legacy files from packages/pwa, packages/pwa-build, and packages/pwa-dev
  • Moved packages/pwa/src/cli contents into packages/pwa-dev
  • Deleted obsolete test fixtures and files that were no longer referenced
  • Reorganized and renamed some test and utility files for clarity
  • Updated references/imports to match the new structure

How to Test-Drive This PR

  1. Clone the branch and install dependencies.
  2. Run npm run test from the root — all tests should still pass 🧪.
  3. Manually inspect the new folder structure and smile in appreciation 😄.

Checklists

General

  • Changes are covered by test cases (no new logic added, existing tests pass)
  • CHANGELOG.md updated with a short description of changes (optional for structural refactors, but included for visibility)

Accessibility Compliance

  • There are no changes to UI

Localization

  • No UI text was updated or added

Let me know if you'd like a pun-filled title suggestion too!

@bendvc bendvc requested a review from a team as a code owner June 30, 2025 22:38
@cc-prodsec
Copy link
Collaborator

cc-prodsec commented Jun 30, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

@bendvc bendvc added the skip changelog Skip the "Changelog Check" GitHub Actions step even if the Changelog.md files are not updated label Jul 1, 2025
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this was added as it wasn't in develop, so I removed it. Things still work without it.

Comment on lines +463 to +469
const printProgramJsonAndExit = async () => {
const output = JSON.stringify(PROGRAM, null, 2)
await new Promise((resolve) => {
process.stdout.write(output + '\n', () => {
resolve()
})
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bug that I originally made where the output was being printed and the program exited before the standard out could be flushed. This fixes that problem.

...parentConfig,
sourceType: 'module'
}
module.exports = require('internal-lib-build/configs/babel.config')
Copy link
Contributor Author

@bendvc bendvc Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Module type was no longer required and our project is not an esm project anymore but we still use import syntax. Its a little weird, but that is how we currently do it in the pwa-kit mono repo. We could choose to change this, but we might have to drop pwa-kit-dev as a source of our jest integration.

* SPDX-License-Identifier: BSD-3-Clause
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/
// eslint-disable-next-line @typescript-eslint/no-var-requires
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing some lint errors now that we have linting for all files working.

'!src/**/*.spec.js'
],
testMatch: ['**/__tests__/**/*.js', '**/?(*.)+(spec|test).js'],
testPathIgnorePatterns: ['bin/*', 'coverage/*', 'dist/*', 'node_modules/*', 'scripts/*'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont test node modules etc.

Comment on lines +6 to +14
"main": "dist/server/server.js",
"files": [
"CHANGELOG.md",
"LICENSE",
"dist/**/*.{js,d.ts}",
"!dist/CHANGELOG.md",
"!dist/README.md",
"!**/*.test.{ts,js}"
],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some clean up on how our package is configured, here we don't want to include test files in our package and some other stuff, we also want to use the built server as our main entry point.

Comment on lines +16 to +17
"build": "cross-env NODE_ENV=production internal-lib-build build",
"build:watch": "nodemon --watch 'src/**' --ext 'js,ts' --exec 'npm run build'",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New build and build watch commands. If you are developing the server use the build watch so that it's automatically built when you make changes to files.

Comment on lines +57 to +60
"engines": {
"node": "^16.11.0 || ^18.0.0 || ^20.0.0 || ^22.0.0",
"npm": "^8.0.0 || ^9.0.0 || ^10.0.0 || ^11.0.0"
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add engines so that it's in alignment with the other projects

Comment on lines +61 to +62
"publishConfig": {
"directory": "dist"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required for when we decide to publish our package.

Comment on lines +19 to +22
// eslint-disable-next-line @typescript-eslint/no-var-requires
const productDocument = require('../data/ProductDocument.json')
// eslint-disable-next-line @typescript-eslint/no-var-requires
const categoryDocument = require('../data/CategoryDocument.json')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we fall back to using require since it works for json files without special syntax that confuses jest

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems weird to use require here. Probably we can convert .json to .js file and export it.

Comment on lines +11 to +13
const BABEL_NODE_PATH = path.resolve(
'./node_modules/.bin/babel-node' + (process.platform === 'win32' ? '.cmd' : '')
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll run the server with babel-node for now so it doesn't complain about the import syntax of the source files. but in the future we migrate to ESM, maybe add out own test setup we can revert this.

*
* This is useful for running CLI tools that emit different output
* when run in a TTY environment (e.g., with color, formatting, or paging).
* Runs a shell command and captures its stdout/stderr as a string.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the 3rd party npm package that I was using to communicate with the create-app script was old and didn't support newer versions of node. So I decided to remove it and use vanilla apis. I can do that because I found the root of the problem for truncated output in the create-app script itself.

import {StdioServerTransport} from '@modelcontextprotocol/sdk/server/stdio.js'

// import {McpServer} from '@modelcontextprotocol/sdk/server/mcp'
// import {StdioServerTransport} from '@modelcontextprotocol/sdk/server/stdio'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems these commented out import should be removed.

@yhsieh1
Copy link
Contributor

yhsieh1 commented Jul 2, 2025

I checked out the branch and tested. No issue found, all commands in package.json runs correctly.

@bendvc bendvc merged commit 91cb2fd into feature/pwa-developer-agent-only Jul 2, 2025
35 checks passed
@bendvc bendvc deleted the bendvc/clean-up-project-structure branch July 16, 2025 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip changelog Skip the "Changelog Check" GitHub Actions step even if the Changelog.md files are not updated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants