Skip to content

feat: npm workspaces #326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
May 30, 2024
Merged

feat: npm workspaces #326

merged 14 commits into from
May 30, 2024

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented May 29, 2024

Rework the monorepo layout to use npm workspaces and per-component package.json files.

TODO:

  • linters
  • CI build
  • Dockerfile(s)
  • Dependabot

Documentation on different install strategies for npm workspaces:
https://docs.npmjs.com/cli/v10/commands/npm-ci?v=true#configuration

Signed-off-by: Miroslav Bajtoš <[email protected]>
bajtos added 3 commits May 29, 2024 10:57
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
with:
node-version: 20
- run: npm ci
- run: npm run lint
Copy link
Member Author

Choose a reason for hiding this comment

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

I need to run standard for all files in the monorepo, including those that are not in any sub-package, e.g. migrations/index.js

Copy link
Member

Choose a reason for hiding this comment

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

Could we turn migrations into a sub-package too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we turn migrations into a sub-package too?

I would prefer indeed if both would work: linting from the parent and the subpackage. Could we create a standard config that re-exports the parent config?

I am not a fan of this. It adds a lot of boilerplate. We will need to add package.json to migrations, add scripts like lint, add standard as a dependency, and so on.

Having written that, I also want to get this monorepo setup finished ASAP and choose the setup that works for both of us, so I am going to take the direction you suggested.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we turn migrations into a sub-package too?

Hmm, we would have to turn bin and scripts into sub-packages too. I definitely don't like that.

I am going to explore a different setup, where the monorepo root is a package on its own, and then some of the subdirectories are packages (workspaces) too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in daa2650

Copy link
Member Author

Choose a reason for hiding this comment

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

I am keeping the lint-all step so that we can lint all workspaces in one go, and in parallel with running the tests.

The alternative is to lint spark-api and spark-publish as part of build-* workflows, and then add another workflow to run only files that are not part of any child workspace (bin, migrations, scripts). I am not sure how easy that would be to do, though, so I prefer the current proposal.

"description": "API for SPARK",
"scripts": {
"start": "node bin/spark.js",
"test": "mocha"
Copy link
Member Author

Choose a reason for hiding this comment

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

In the main branch, one could run (cd spark-publish && npm test) to execute both the linter and the tests for a sub-package.

Is this something you want to preserve in the new monorepo setup?

In my proposal, you always have to run the linter from the monorepo root, because that's the place where package.json tells standard about Mocha.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer indeed if both would work: linting from the parent and the subpackage. Could we create a standard config that re-exports the parent config?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in daa2650

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we create a standard config that re-exports the parent config?

I could not find any quick way how to do that, Standard seems to accept config only via CLI args or in the closest package.json file.

The alternative is to eject from standard to eslint, but that's much more work than I am willing to do now.

We could also remove the env config from package.json and modify every file to start with the following line:

/* eslint-env mocha */

It would be cleaner because non-test files won't have access to describe & it globals, but it also feels like too much boilerplate.

Copy link
Member Author

Choose a reason for hiding this comment

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

I ended up duplicating the "standard":{"env":...}} section in each package.json.

Signed-off-by: Miroslav Bajtoš <[email protected]>
Copy link
Member

@juliangruber juliangruber left a comment

Choose a reason for hiding this comment

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

This setup seems to work well for me - but I haven't stress tested it yet

with:
node-version: 20
- run: npm ci
- run: npm run lint
Copy link
Member

Choose a reason for hiding this comment

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

Could we turn migrations into a sub-package too?

"description": "API for SPARK",
"scripts": {
"start": "node bin/spark.js",
"test": "mocha"
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer indeed if both would work: linting from the parent and the subpackage. Could we create a standard config that re-exports the parent config?

bajtos added 2 commits May 29, 2024 15:43
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
bajtos added 3 commits May 29, 2024 16:08
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
@bajtos bajtos marked this pull request as ready for review May 29, 2024 14:16
Signed-off-by: Miroslav Bajtoš <[email protected]>
@bajtos bajtos merged commit bdbe7e9 into monorepo May 30, 2024
8 checks passed
@bajtos bajtos deleted the monorepo-workspaces branch May 30, 2024 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants