Skip to content

Conversation

@aurelien-reeves
Copy link
Contributor

@aurelien-reeves aurelien-reeves commented Feb 19, 2021

Summary

Binging NPM 7 Workspaces into the monorepo

Details

Refs. #1259

TODO

  • Set up npm workspaces and tsconfig.build.json so we can npm i && npm run build all npm modules from root.
  • Make npm run eslint-fix work from root
  • Make npm run upgrade work from root
  • Remove common dev dependencies and configs from */javascript/package.json and only declare them once in the root package.json
  • Remove "build", "lint" and "lint-fix" from all */javascript/package.json files (should only be in the root one).
  • Ensure that the npm prepare lifecycle script won't run when consumers install the package
  • Update .templates/javascript/default.mk
  • Remove .templates/javascript/.eslintrc.json and all copies
  • Remove .templates/javascript/.prettierrc and all copies
  • Figure out what to do with the generated messages/javascript/src/messages.js and messages/javascript/src/messages.d.ts files
    • Currently, npm i && npm run build will fail from a fresh clone. We don't want to have to do npm i && pushd messages/javascript && make && popd && npm run build
  • Allow contributors to run TypeScript code without needing to build the code (no need to run npm run build from the root)
    • Update all tsconfig.json files to just { "extends": "../../tsconfig.json" }
    • Set up compilerOptions.paths in root tsconfig.json file
    • Set up tsconfig-paths
  • Update .circleci/config.yml
    • Can we still build npm modules independently?
    • If yes, do they need to depend on a job that does the npm install && npm run build?
    • What's left to do in npm module CI jobs? Just npm test?
  • Update the monorepo build
    • Need to tun npm i in root. Running npm run build should be the last step. Run npm test to test all modules.
  • Update contributor docs
  • Update .templates/javascript/default.mk
    • Remove remove-local-dependencies
  • Decide what to put in ./tsconfig.base.json and ./tsconfig.build.json - at the moment some duplication remains in those files
  • Add missing files (.rsync) to the new packages (message-streams, gherkin-streams)
  • Decide whether we should move stream classes for other implementations as well (to remain consistent)
  • Make sure gherkin acceptance tests are running

@davidjgoss
Copy link
Contributor

davidjgoss commented Feb 19, 2021

Figure out what to do with the generated messages/javascript/src/messages.js and messages/javascript/src/messages.d.ts files...Currently, npm i && npm run build will fail from a fresh clone. We don't want to have to do npm i && pushd messages/javascript && make && popd && npm run build

In the messages/javascript package.json, try adding:

{
  "scripts": {
    "prepare": "npm run pbjs && npm run pbts",
    ...
  }
}

That should run automatically after install in a local/workspace context. See https://docs.npmjs.com/cli/v7/using-npm/scripts#life-cycle-scripts.

@aslakhellesoy
Copy link
Contributor

Figure out what to do with the generated messages/javascript/src/messages.js and messages/javascript/src/messages.d.ts files...Currently, npm i && npm run build will fail from a fresh clone. We don't want to have to do npm i && pushd messages/javascript && make && popd && npm run build

In the messages/javascript package.json:

{
  "scripts": {
    "prepare": "npm run pbjs && npm run pbts",
    ...
  }
}

That should run automatically after install in a local/workspace context. See https://docs.npmjs.com/cli/v7/using-npm/scripts#life-cycle-scripts.

My understanding of npm workspaces is that you're only supposed to run npm install from the root directory. I wonder if this skips running of individual modules' npm lifecycle scripts.

Any idea?

@davidjgoss
Copy link
Contributor

davidjgoss commented Feb 19, 2021

Looks like the answer is "we fixed it recently" npm/cli#1965.

I'll push a commit and we find out?

so we generate the code from the protobuf definition right after install
@davidjgoss
Copy link
Contributor

davidjgoss commented Feb 19, 2021

Worked for me locally with a fresh clone! Also npm 7 is very fast!

dist/src/styles/cucumber-react.css: src/styles/styles.scss src/styles/react-accessible-accordion.css
mkdir -p $(@D)
./node_modules/.bin/sass $< > $@
../../node_modules/.bin/sass $< > $@
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about using npx sass instead of a relative path to the binary?

Copy link
Contributor

Choose a reason for hiding this comment

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

and/or could we move to the package.json, maybe another candidate for using prepare?

- make sure .codegen happens in react when `npm install`
- make sure to copy `messages.js` and `messages.d.ts` in `dist` in
  messages when `npm install`
"declaration": true,
"declarationMap": true,
"sourceMap": true,
"target": "es6",
Copy link
Contributor

Choose a reason for hiding this comment

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

Given Node 10 is our lowest common denominator, we should be able to target es2018 - means no transpilation of async/await etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although I guess we need to ensure some packages work in browser. Do we leave it up to consuming projects to do transpilation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Given Node 10 is our lowest common denominator

Is it though? Some modules also need to work in the browser, such as cucumber/@react and all its dependencies. I also know that some teams use cucumber/@gherkin in the browser - I recently decoupled node-specific libraries to make this possible without polyfills.

So with that in mind, what do you think we should target?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I remembered about browsers right after I hit Comment :)

I'd like to think we could just ship ES[current] code and have consuming projects do the transpiling they need, but I don't think the ecosystem is there yet. So:

  • es6 assuming we aren't worried about IE
  • es5 if we do need to worry about IE

@aslakhellesoy aslakhellesoy added language: javascript 🔧 build Related to build / release process labels Feb 22, 2021
@bbros-dev
Copy link

The details provided by @aslakhellesoy here and @davidjgoss, here, would be useful inclusions in the existing item:

  • Update contributor docs

@aurelien-reeves aurelien-reeves marked this pull request as ready for review March 10, 2021 12:47
@aslakhellesoy aslakhellesoy merged commit 131ced3 into master Mar 10, 2021
@aslakhellesoy aslakhellesoy deleted the npm-workspaces branch March 10, 2021 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

language: javascript 🔧 build Related to build / release process

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants