Set type: "module" for esm/ and test/ in all packages & update to mocha@9 #567

eemeli · 2021-07-21T14:27:47Z

While the top-level package.json files include a "type": "commonjs" specification, the esm/ directories can (and ought) to either have their contents use the .mjs extension, or include their own package.json file with the correct "type".

This adds the package.json files while keeping the extensions as .js, as this is significantly easier for the TypeScript interop; proper .mjs support is currently an unresolved issue.

~~For context, I noticed/encountered this issue while updating Mocha to its latest version; I'll file another PR on that separately if/once this is merged.~~

Edit: Mocha update is now included in this PR, as the old version wasn't able to handle the "module" types.

While the top-level package.json files include a "type": "commonjs" specification, the esm/ directories can (and ought) to either have their contents use the .mjs extension, or include their own package.json file with the correct "type". This adds the package.json while keeping the extensions as .js, as this is significantly easier for the TypeScript interop; proper .mjs support is currently an unresolved issue: microsoft/TypeScript#18442

eemeli · 2021-07-21T14:37:07Z

Bother, it looks like the Mocha update needs to be done simultaneously. I'll include that here & rephrase the description.

While extensionless imports are supported by TypeScript as well as the esm package, the extensions are required by Node.js when resolving imports. TypeScript's tsc is also completely fine with using .js extensions when referring to .ts files.

gregtatum

Setting as "Request changes" until the mocha issue is resolved. Please re-request review from me when you are ready.

The consideration of test/ files as ES modules requires a few of them to use import.meta, which isn't supported by the default parser of eslint@6. So this is a minimal patch to get linting to continue working until ESLint itself is updated.

eemeli · 2021-07-22T10:32:12Z

Tooling updates are fun. Also needed to patch the ESLint configs, which to be clear are explicitly using babel-eslint rather than @babel/eslint-parser, as the latter requires a newer ESLint version.

Each of the commits should explain themselves. I'll submit the actual ESLint update separately, as that looks like it'll require more changes to account for changed recommendations.

Demivan · 2021-07-22T12:37:28Z

fluent-syntax/src/index.ts

+export * from "./ast.js";
+export * from "./errors.js";
+export * from "./parser.js";
+export * from "./serializer.js";
+export * from "./visitor.js";


Why does it import .js files from .ts file?

Because this is the cleanest way of producing valid ES module code in the esm/ directories. Node.js resolution requires the .js extension for import calls, and TypeScript automatically resolves e.g. ./ast.js as ./ast.ts.

Without these, we'd need a separate build step after the tsc call that would modify all of the relative paths to use fully qualified extensions. It'd be nice if TypeScript supported that natively, but, well, it doesn't. This is also why we need the esm/package.json files, as TypeScript only handles the .js extension like this, and not e.g. .mjs.

gregtatum

The changes all look reasonable to me, and CI is passing, so I'm happy to give my approval. Thanks for the maintenance work! I left one optional suggestion, and a few minor comments.

gregtatum · 2021-07-22T19:23:42Z

fluent-bundle/test/arguments_test.js

-import {FluentBundle} from '../esm/bundle';
-import {FluentResource} from '../esm/resource';
-import {FluentType, FluentNumber, FluentDateTime} from '../esm/types';
+import {FluentBundle} from '../esm/bundle.js';


Question: If someone regresses this by changing it back to '../esm/bundle' will our CI catch it?

Yes, the Mocha tests will fail then. The exact error they fail with is a bit misleading though:

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/eemeli/code/fluent-js/fluent-bundle/test/arguments_test.js require() of ES modules is not supported. require() of /Users/eemeli/code/fluent-js/fluent-bundle/test/arguments_test.js from /Users/eemeli/code/fluent-js/node_modules/mocha/lib/esm-utils.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules. Instead rename arguments_test.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/eemeli/code/fluent-js/fluent-bundle/test/package.json.

There's a try { ... } catch block around the import() here which falls back to require(), and logs its error if both fail. I may end up filing an upstream bug about this.

gregtatum · 2021-07-22T19:25:50Z

package-lock.json

        "nyc": "^15.1.0",
        "prettyjson": "^1.2.1",
        "rollup": "^1.9.3",
        "typedoc": "^0.20.34",
        "typescript": "^4.2.3"
      },
      "engines": {
-        "node": ">=10.0.0"
+        "node": ">=12.0.0"


Thought: I double checked that 12 is the current LTS, so this change looks good to me.

Yeah, this is a result of the changes in #557 which weren't completely reflected in the lockfiles.

gregtatum · 2021-07-22T19:28:18Z

eslint_test.json

@@ -1,4 +1,5 @@
 {
+    "parser": "babel-eslint",


Suggestion (optional): It would be nice to document why this parser is needed in case it can be removed, however this is a JSON configuration file which doesn't support comments. I'm not sure if it's feasible to convert it to a .js configuration file, so I'm marking this suggestion as optional.

Ah, I believe this comment is outdated by your next PR which converts it to .yml.

This dependency gets updated to @babel/eslint-parser by #568. That PR also ports the ESLint configs to YAML in order to allow them to be later commented.

eemeli requested a review from gregtatum July 21, 2021 14:27

eemeli added 3 commits July 21, 2021 17:38

test(fluent-dom): Move elem() from index.js to util.js

3497500

test: Update to mocha@9, now using Node.js native import

f18aa2b

gregtatum requested changes Jul 21, 2021

View reviewed changes

eemeli changed the title ~~Add an esm/package.json to all packages with { "type": "module" }~~ Set type: "module" for esm/ and test/ in all packages & update to mocha@9 Jul 22, 2021

eemeli requested a review from gregtatum July 22, 2021 10:29

Demivan reviewed Jul 22, 2021

View reviewed changes

eemeli mentioned this pull request Jul 22, 2021

Update ESLint and simplify its configs #568

Merged

gregtatum approved these changes Jul 22, 2021

View reviewed changes

eemeli merged commit d874a75 into projectfluent:master Jul 22, 2021

eemeli deleted the add-esm-packages branch July 22, 2021 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set type: "module" for esm/ and test/ in all packages & update to mocha@9 #567

Set type: "module" for esm/ and test/ in all packages & update to mocha@9 #567

eemeli commented Jul 21, 2021 •

edited

Loading

eemeli commented Jul 21, 2021

gregtatum left a comment

eemeli commented Jul 22, 2021

Demivan Jul 22, 2021

eemeli Jul 22, 2021

gregtatum left a comment

gregtatum Jul 22, 2021

eemeli Jul 22, 2021

gregtatum Jul 22, 2021

eemeli Jul 22, 2021

gregtatum Jul 22, 2021

gregtatum Jul 22, 2021

eemeli Jul 22, 2021

Set type: "module" for esm/ and test/ in all packages & update to mocha@9 #567

Set type: "module" for esm/ and test/ in all packages & update to mocha@9 #567

Conversation

eemeli commented Jul 21, 2021 • edited Loading

eemeli commented Jul 21, 2021

gregtatum left a comment

Choose a reason for hiding this comment

eemeli commented Jul 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eemeli commented Jul 21, 2021 •

edited

Loading