Add npm workspaces & tool-specific config files, drop make #569

eemeli · 2021-07-28T10:09:07Z

Apologies for the size of this PR, but the whole changeset seemed best kept together. This transitions the repo to use and depend on npm workspaces for package management. This is what that means for each of the previous make commands, as run from the repo root:

`make deps` and `make depsclean`

Completely dropped. Running npm install or npm ci at the root now installs all dependencies of all packages. Per-package lockfiles are also all dropped.

`make lint` → `npm run lint`

Run now only from the repo root, linting all packages at once. Due to updated ESLint rules this will now complain if the packages have not been built first, as otherwise missing transitive dependencies resolve as any.

`make test` → `npm test`

The common Mocha config allows testing 5/8 packages all at once (via npm run test:common). @fluent/dom gets a separate invocation due to its JSDOM setup, and @fluent/react on account of using Jest. @fluent/gecko remains without tests, but it's just repackaging other bits. Tests continue to use built rather than source assets. A .mocharc.yaml config file is added to the repo root.

Internally, the code coverage tool is switched from nyc to c8, as the latter works more dependably with ES modules as used by mocha@9. The output is the same for both, so this has no external visibility.

`make build` → `npm run build -ws`

Note the -ws argument: that's short for --workspaces, and it runs the build command in all of the packages, in the order specified in the "workspaces" array of the root package.json file. The order matters, as npm doesn't automatically sort the packages topologically based on their internal dependencies. Rollup configs are now in rollup.config.mjs files in the repo root and in fluent-gecko/, rather than as CLI arguments inside makefiles.

`make clean` → `npm run clean`

Run now only from the repo root, cleaning all packages at once. Uses git clean, as that's smart enough to only remove files that aren't stored in the repo.

`make html` → `npm run docs -ws`

Config is moved from makefiles to typedoc.config.cjs in the repo root.

`make dist` → `npm run dist`

Defined now by the "predist", "dist" and "postdist" scripts in the root package.json file. Previously this looped packages/commands; now it's commands/packages.

Other notable changes

Each package provides its own build, test and docs scripts, so that they may be run individually.
CI actions are collapsed into two: test running on Node.js 16 and legacy for Node.js 12 & 14. Each of these essentially runs the dist command, but the older versions need a preliminary npm i -g npm@7 call as on GitHub they initially come with npm v6.
A few makefiles are left in, with targets operating on fixtures and running perf commands.

If updating a previous installation past this PR, it's best to first run rm -rf fluent-*/node_modules, as all of their previous contents should now be found in the root node_modules directory, and keeping them will cause weird problems down the line.

…kfiles

nyc was no longer working with the ES module imports, and c8 is in any case more efficient, using native V8 coverage made available in Node.js 10.12.0 and later.

…g them during build

…ncies

…rkflows As the workspaces config requires npm@7, that needs to be installed separately for Node.js 12 & 14 in GitHub Actions.

eemeli · 2021-07-28T10:32:43Z

Oops, I just realised that the preceding doesn't say why I'd like to do this. In short, this would make the repository's structure far less custom and more "normal" in the context of JavaScript projects. In particular, the workspace commands make it much easier to maintain the repo going forward, e.g. updating dependencies and making other changes. Using config files should make command configuration more accessible, as familiarity with make is no longer required.

The restructuring also allows for linting in particular to be only run once, speeding it up about 2x for all packages. The CI runtime is longer, as it's no longer parallelised 8x. About 1/3 of that time is spent in typedoc.

gregtatum · 2021-07-29T21:12:41Z

I'm on board in general with these changes, but just a bit bogged down with planning this week, so haven't gotten a chance to review.

gregtatum

Overall the changes look much easier to maintain, especially for someone who is familiar with npm-based workflows.

I'm marking as request changes as many of the commands won't run on a clean installation.

I left a few other minor comments around documenting a few things.

I double checked the CI on this PR, and I believe we're probably good on not breaking the CI.

gregtatum · 2021-08-10T14:01:56Z

package.json

  "private": true,
+  "workspaces": [


praise: This is much cleaner.

gregtatum · 2021-08-10T14:02:45Z

README.md

-Install dependencies of individual `fluent.js` packages which are required for
-passing tests:
-
-    $ make deps


praise: Thanks, this nicer not have to deal with make for installing dependencies.

fluent-react/makefile

gregtatum · 2021-08-10T14:09:46Z

makefile

@@ -2,7 +2,9 @@ export SHELL := /bin/bash
 GH_PAGES ?= $(CURDIR)/node_modules/.bin/gh-pages

 TARGETS  := all dist test build html
-PACKAGES := $(wildcard fluent-*)
+
+# The order matters: dedent > bundle > sequence > others


suggestion: It's unclear to me why the order matters. It would be good to document the "why" here.

question: Is there a risk that a new package will fail to register here?

The order matters because the inter-package dependencies end up referring to the compiled versions, and then the builds fail when missing type definitions fall back to any values if the root of the dependency tree doesn't come before the leaves.

This order eventually ends up in the package.json workspaces list, so I think the risk is minimal: a package needs to be there in order for anything with it to really work.

rollup.config.mjs

gregtatum · 2021-08-10T14:18:51Z

.github/workflows/legacy.yml

 jobs:
  dist:
    runs-on: ubuntu-latest
    strategy:
      matrix:
-        node: [12.x, 14.x, 16.x]
+        node: [12.x, 14.x]


question (blocking): How are you manually testing the CI changes. I'd like to ensure this works before it's merged in.

I ran the tests here: https://github.com/eemeli/fluent.js/actions

I also added workflow_dispatch to the on map (line 7), which enables manually running the workflow.

gregtatum · 2021-08-11T14:05:03Z

package.json

@@ -12,7 +12,11 @@
    "./fluent-gecko"
  ],
  "scripts": {
+    "predist": "npm run clean",


Suggestion (follow-up, optional): npm run scripts don't allow for inline comments, but it would be nice to document the intended use of these scripts for contributors and maintainers. The README.md is a good start for running them.

gregtatum · 2021-08-11T14:24:12Z

package.json

+    "deploy-html": "gh-pages -d html",
+    "lint": "eslint --max-warnings 0 'fluent-*/{src,test}/**/*.{js,ts}'",
+    "test:common": "c8 mocha 'fluent-*/test/*_test.js'",
+    "test": "npm run test:common && npm test -w fluent-dom && npm test -w fluent-react"


Issue: This fails for me locally:
I'm running npm 7.7.6.

~/dev/fluent.js on fluent.js/pr/569 at 09:21:31 ➤ npm test > test > npm run test:common && npm test -w fluent-dom && npm test -w fluent-react > test:common > c8 mocha 'fluent-*/test/*_test.js' Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/greg/dev/fluent.js/fluent-langneg/esm/accepted_languages.js' imported from /Users/greg/dev/fluent.js/fluent-langneg/test/headers_test.js at new NodeError (node:internal/errors:329:5) at finalizeResolution (node:internal/modules/esm/resolve:323:11) at moduleResolve (node:internal/modules/esm/resolve:758:10) at Loader.defaultResolve [as _resolve] (node:internal/modules/esm/resolve:869:11) at Loader.resolve (node:internal/modules/esm/loader:88:40) at Loader.getModuleJob (node:internal/modules/esm/loader:241:28) at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:57:40) at link (node:internal/modules/esm/module_job:56:36) -----------------------------|---------|----------|---------|---------|----------------------------- File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s -----------------------------|---------|----------|---------|---------|----------------------------- All files | 36.79 | 99.15 | 0 | 36.79 | fluent-bundle/esm | 33.02 | 100 | 0 | 33.02 | builtins.js | 70.55 | 100 | 0 | 70.55 | 14-22,62-78,130-146 bundle.js | 64.38 | 100 | 0 | 64.38 | ...-59,70-71,91-112,141-159 resolver.js | 16.48 | 100 | 0 | 16.48 | ...,213-226,229-264,267-273 resource.js | 11.44 | 100 | 0 | 11.44 | 47-430,434-436 scope.js | 15.15 | 100 | 0 | 15.15 | 3-14,16-20,22-32 types.js | 70.37 | 100 | 0 | 70.37 | ...58-60,65-73,92-94,99-107 fluent-bundle/test | 37.74 | 100 | 0 | 37.74 | arguments_test.js | 45.42 | 100 | 0 | 45.42 | ...,218-222,231-232,236-259 attributes_test.js | 41.21 | 100 | 100 | 41.21 | ...,173-182,186-189,193-196 bomb_test.js | 49.02 | 100 | 100 | 49.02 | 13,18-31,35-39,43-48 constructor_test.js | 42.31 | 100 | 100 | 42.31 | 12,16,20-33,37-50 context_test.js | 43.66 | 100 | 100 | 43.66 | ...,120-124,128-132,136-138 errors_test.js | 43.48 | 100 | 100 | 43.48 | 13-16,20-35,39-44 functions_builtin_test.js | 11.36 | 100 | 100 | 11.36 | ...,477-509,513-548,552-587 functions_runtime_test.js | 57.45 | 100 | 100 | 57.45 | 13,18-28,32-35,41-44 functions_test.js | 48.15 | 100 | 100 | 48.15 | ...9-82,88-91,95-98,102-105 isolating_test.js | 52.13 | 100 | 100 | 52.13 | ...-59,63-69,77-81,85,89-92 literals_test.js | 35 | 100 | 100 | 35 | ...,30-39,43-52,56-65,69-78 macros_test.js | 39.53 | 100 | 100 | 39.53 | ...,410-413,417-420,424-427 object_prototype_test.js | 46.86 | 100 | 100 | 46.86 | ...,217-221,225-229,233-236 parser_reference_test.js | 87.1 | 100 | 100 | 87.1 | 25-28 parser_structure_test.js | 87.1 | 100 | 100 | 87.1 | 25-28 patterns_test.js | 43.23 | 100 | 100 | 43.23 | ...,196-212,216-219,223-226 primitives_test.js | 40.68 | 100 | 100 | 40.68 | ...,156-159,163-166,170-173 select_expressions_test.js | 33.57 | 100 | 100 | 33.57 | ...,101-110,114-123,127-136 transform_test.js | 46.77 | 100 | 100 | 46.77 | ...,34-39,43-46,50-53,57-60 values_format_test.js | 47.44 | 100 | 100 | 47.44 | ...,50-53,57-60,64-67,71-74 values_ref_test.js | 32.65 | 100 | 100 | 32.65 | ...,115-124,128-134,138-144 fluent-dedent | 43.64 | 75 | 0 | 43.64 | index.js | 43.64 | 75 | 0 | 43.64 | 4,22-51 fluent-dedent/esm | 31.82 | 100 | 0 | 31.82 | index.js | 31.82 | 100 | 0 | 31.82 | 15-44 fluent-dedent/test | 40.68 | 100 | 100 | 40.68 | blank_test.js | 37.5 | 100 | 100 | 37.5 | ...,29-35,39-44,48-53,57-62 content_test.js | 37.5 | 100 | 100 | 37.5 | ...,18-24,28-34,38-44,48-54 eol_test.js | 47.37 | 100 | 100 | 47.37 | 8-11,15-19,23-27,31-36 interpolation_test.js | 42.86 | 100 | 100 | 42.86 | 8-13,17-22,26-33 mixed_test.js | 45.45 | 100 | 100 | 45.45 | 8-13,17-22,26-31 tabs_test.js | 39.13 | 100 | 100 | 39.13 | ...,35-40,44-49,53-58,62-67 -----------------------------|---------|----------|---------|---------|-----------------------------

After running npm clean I get a different error:

➤ npm test > test > npm run test:common && npm test -w fluent-dom && npm test -w fluent-react > test:common > c8 mocha 'fluent-*/test/*_test.js' Error [ERR_MODULE_NOT_FOUND]: Cannot find package '/Users/greg/dev/fluent.js/node_modules/@fluent/dedent/' imported from /Users/greg/dev/fluent.js/fluent-bundle/test/arguments_test.js at new NodeError (node:internal/errors:329:5) at legacyMainResolve (node:internal/modules/esm/resolve:272:9) at packageResolve (node:internal/modules/esm/resolve:707:14) at moduleResolve (node:internal/modules/esm/resolve:755:18) at Loader.defaultResolve [as _resolve] (node:internal/modules/esm/resolve:869:11) at Loader.resolve (node:internal/modules/esm/loader:88:40) at Loader.getModuleJob (node:internal/modules/esm/loader:241:28) at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:57:40) at link (node:internal/modules/esm/module_job:56:36) ----------|---------|----------|---------|---------|------------------- File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s ----------|---------|----------|---------|---------|------------------- All files | 0 | 0 | 0 | 0 | ----------|---------|----------|---------|---------|-------------------

Suggestion: After running npm dist this now works. I feel that the command should be more robust.

I also ran into this: #572

Ok, I finally got the tests passing locally after commenting out the failing test.

I was thinking while working on this that the tests ought to depend on the source rather than dist files. I left that change out of this PR because I felt it to be a reasonable future step.

Similarly the build-time tsc dependencies ought to be mapped to their dependencies' sources, but that too is a separate change.

Together, these should make individual scripts far less dependent on being executed in the "correct" order, and for the order of entries in the package.json workspaces to matter less.

gregtatum · 2021-08-11T14:26:16Z

package.json

+    "postdist": "npm run lint && npm run test && npm run docs --workspaces",
+    "clean": "git clean -fdxe node_modules fluent-*/{coverage,dist,esm,index.js} html",
+    "deploy-html": "gh-pages -d html",
+    "lint": "eslint --max-warnings 0 'fluent-*/{src,test}/**/*.{js,ts}'",


Issue: I get failing lints, it seems that this call should be clean.

➤ npm run lint > lint > eslint --max-warnings 0 'fluent-*/{src,test}/**/*.{js,ts}' /Users/greg/dev/fluent.js/fluent-react/src/localization.ts 30:5 error Unsafe return of an `any` typed value @typescript-eslint/no-unsafe-return 30:12 error Unsafe call of an `any` typed value @typescript-eslint/no-unsafe-call 44:11 error Unsafe assignment of an `any` value @typescript-eslint/no-unsafe-assignment 46:13 error Unsafe assignment of an `any` value @typescript-eslint/no-unsafe-assignment 46:19 error Unsafe member access .getMessage on an `any` value @typescript-eslint/no-unsafe-member-access 46:19 error Unsafe call of an `any` typed value @typescript-eslint/no-unsafe-call 47:18 error Unsafe member access .value on an `any` value @typescript-eslint/no-unsafe-member-access 49:13 error Unsafe assignment of an `any` value @typescript-eslint/no-unsafe-assignment 49:21 error Unsafe member access .formatPattern on an `any` value @typescript-eslint/no-unsafe-member-access 49:21 error Unsafe call of an `any` typed value @typescript-eslint/no-unsafe-call 49:42 error Unsafe member access .value on an `any` value @typescript-eslint/no-unsafe-member-access 53:9 error Unsafe return of an `any` typed value @typescript-eslint/no-unsafe-return /Users/greg/dev/fluent.js/fluent-react/src/localized.ts 75:9 error Unsafe assignment of an `any` value @typescript-eslint/no-unsafe-assignment 106:9 error Unsafe assignment of an `any` value @typescript-eslint/no-unsafe-assignment 106:15 error Unsafe member access .getMessage on an `any` value @typescript-eslint/no-unsafe-member-access 106:15 error Unsafe call of an `any` typed value @typescript-eslint/no-unsafe-call 113:9 error Unsafe member access .value on an `any` value @typescript-eslint/no-unsafe-member-access 115:11 error Unsafe assignment of an `any` value @typescript-eslint/no-unsafe-assignment 115:19 error Unsafe member access .formatPattern on an `any` value @typescript-eslint/no-unsafe-member-access 115:19 error Unsafe call of an `any` typed value @typescript-eslint/no-unsafe-call 115:40 error Unsafe member access .value on an `any` value @typescript-eslint/no-unsafe-member-access 130:16 error Unsafe member access .attributes on an `any` value @typescript-eslint/no-unsafe-member-access 134:30 error Unsafe member access .attributes on an `any` value @typescript-eslint/no-unsafe-member-access 135:9 error Unsafe assignment of an `any` value @typescript-eslint/no-unsafe-assignment 135:32 error Unsafe member access .formatPattern on an `any` value @typescript-eslint/no-unsafe-member-access 135:32 error Unsafe call of an `any` typed value @typescript-eslint/no-unsafe-call 136:11 error Unsafe member access .attributes on an `any` value @typescript-eslint/no-unsafe-member-access 155:7 error Unsafe member access .value on an `any` value @typescript-eslint/no-unsafe-member-access 160:9 error Unsafe assignment of an `any` value @typescript-eslint/no-unsafe-assignment 160:24 error Unsafe member access .formatPattern on an `any` value @typescript-eslint/no-unsafe-member-access 160:24 error Unsafe call of an `any` typed value @typescript-eslint/no-unsafe-call 160:45 error Unsafe member access .value on an `any` value @typescript-eslint/no-unsafe-member-access /Users/greg/dev/fluent.js/fluent-sequence/src/map_async.ts 26:11 error Unsafe member access .hasMessage on an `any` value @typescript-eslint/no-unsafe-member-access 26:11 error Unsafe call of an `any` typed value @typescript-eslint/no-unsafe-call 27:9 error Unsafe return of an `any` typed value @typescript-eslint/no-unsafe-return 39:35 error Unsafe member access .hasMessage on an `any` value @typescript-eslint/no-unsafe-member-access 39:35 error Unsafe call of an `any` typed value @typescript-eslint/no-unsafe-call 40:9 error Unsafe assignment of an `any` value @typescript-eslint/no-unsafe-assignment 46:9 error Unsafe return of an `any[]` typed value @typescript-eslint/no-unsafe-return 51:3 error Unsafe return of an `any[]` typed value @typescript-eslint/no-unsafe-return /Users/greg/dev/fluent.js/fluent-sequence/src/map_sync.ts 29:11 error Unsafe return of an `any` typed value @typescript-eslint/no-unsafe-return 41:9 error Unsafe member access .hasMessage on an `any` value @typescript-eslint/no-unsafe-member-access 41:9 error Unsafe call of an `any` typed value @typescript-eslint/no-unsafe-call 42:7 error Unsafe return of an `any` typed value @typescript-eslint/no-unsafe-return ✖ 44 problems (44 errors, 0 warnings)

Does running the dist script before lint solve this for you? This is almost certainly caused by inter-package dependencies using compiled rather than source dependencies.

npm run lint is working now, so it must have relied on dist.

Suggestion: It seems like the run scripts are failing due to dependency management. Makefiles can run dependent jobs correctly, so it might be worth adding a makefile into the mix to ensure the commands are easy to run. The makefiles could still call npm run scripts.

I would actually prefer to solve this by removing the dependencies on the dist assets, but that seems like a separate change from starting to use npm workspaces so I've not included it here.

gregtatum · 2021-08-11T14:35:30Z

package.json

+    "dist": "npm run build --workspaces",
+    "postdist": "npm run lint && npm run test && npm run docs --workspaces",
+    "clean": "git clean -fdxe node_modules fluent-*/{coverage,dist,esm,index.js} html",
+    "deploy-html": "gh-pages -d html",


Issue: This one failed too:

➤ npm run deploy-html > deploy-html > gh-pages -d html ENOENT: no such file or directory, stat '/Users/greg/dev/fluent.js/html'

I think we need a mkdir html here.

The html directory is generated by the packages' docs scripts, which are run as a part of postdist. Running this script should then update your local gh-pages branch and push that to GitHub.

So it's probably a good thing that this failed for you, as on success it would've updated the contents that we're serving under https://projectfluent.org/fluent.js/.

eemeli · 2021-08-25T15:51:43Z

@gregtatum Do you have remaining concerns about this PR, or have they been resolved? As in, would it be okay to merge this in first, and then separately work on removing the (already existing, but here more explicitly revealed) dependencies between the packages' build order?

gregtatum

Yes if you're comfortable landing the changes then it's fine with me. Thanks for the ping on this, I typically review things based on the review status. So if you can mark me explicitly for re-review I'll take another look at patches, otherwise it gets a bit lost in the github notification spam. I happened to catch up github notifications today and saw your latest comment.

eemeli added 15 commits July 27, 2021 16:46

refactor: Add workspaces config to root package.json, merging all loc…

6582225

…kfiles

chore: Update scripts & docs to reflect config changes

d996498

chore: Fix ESLint config to allow running from root

11ea957

chore: Add npm test scripts for the root & all packages

86f6ce3

chore: Switch from nyc to c8 for code coverage

b7bf2e8

nyc was no longer working with the ES module imports, and c8 is in any case more efficient, using native V8 coverage made available in Node.js 10.12.0 and later.

chore: Include esm/package.json files in repo, rather than re-creatin…

0b27f66

…g them during build

chore: Enforce correct order for package operations, based on depende…

e6f8b57

…ncies

chore: Add npm run build scripts, using a common Rollup config file

3221fcb

chore: Add common config & npm scripts for typedoc

27c9165

chore: Add a npm run clean script to the root, using git clean

c50e90d

chore(gecko): Merge Rollup configs

2e8bdcf

ci: Merge separate package workflows into common "test" & "legacy" wo…

e0d2335

…rkflows As the workspaces config requires npm@7, that needs to be installed separately for Node.js 12 & 14 in GitHub Actions.

chore: Add npm run dist and npm run deploy-html scripts

8262a01

chore: Drop obsolete makefiles

2ded43e

docs: Use npm scripts rather than make, mention npm v7 requirement

c09e7aa

eemeli requested review from mathjazz and gregtatum July 28, 2021 10:09

gregtatum requested changes Aug 11, 2021

View reviewed changes

gregtatum self-requested a review August 30, 2021 14:51

gregtatum approved these changes Aug 30, 2021

View reviewed changes

eemeli merged commit afa3897 into projectfluent:master Sep 13, 2021

eemeli deleted the workspacing branch September 13, 2021 07:58

This was referenced Sep 13, 2021

Improve the TypeScript lint/intellisense experience #573

Merged

make fluent-react.js in fluent-gecko fails #523

Closed

Repository managment #275

Closed

Add npm workspaces & tool-specific config files, drop make #569

Add npm workspaces & tool-specific config files, drop make #569

Uh oh!

Conversation

eemeli commented Jul 28, 2021

make deps and make depsclean

make lint → npm run lint

make test → npm test

make build → npm run build -ws

make clean → npm run clean

make html → npm run docs -ws

make dist → npm run dist

Other notable changes

Uh oh!

eemeli commented Jul 28, 2021

Uh oh!

gregtatum commented Jul 29, 2021

Uh oh!

gregtatum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eemeli commented Aug 25, 2021

Uh oh!

gregtatum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

`make deps` and `make depsclean`

`make lint` → `npm run lint`

`make test` → `npm test`

`make build` → `npm run build -ws`

`make clean` → `npm run clean`

`make html` → `npm run docs -ws`

`make dist` → `npm run dist`