feat: convert web-ext package into a nodejs native ES module #2405

rpl · 2022-05-31T08:44:03Z

Fixes #2311
Fixes #1306

This pull requests goal is to convert web-ext package into a nodejs native ES module.

Changes to npm scripts to work as ESM modules (and replaced webpack with babel-cli as build step)
Add small custom node module loader (tests/babel-loader.js) to transpile on the fly (used to make mocha able to load tests modules using flowtype syntaxes)
Re-export custom node module loader exports provided by testdouble / quibble to be able to mock ES modules in some tests (used as part of workaround for sinon.stub limitations on stubbing ES modules)
Added babel-plugin-transform-inline-environment-variables to interpolate environment variable during babel transpiling (replaces webpack interpolated WEBEXT_BUILD_ENV global)
converted build scripts and fake firefox and amo scripts to ES modules
fixed sign-addon import when it is loaded using the nodejs native ES module loader (caught by functional tests)

package size comparison

The new package size is 165 kB (unpacked 699 kB) vs old package size 103 kB (unpacked 442 kB).

TODO list

fix unexpected failures while running unit test on the windows CI worker
consider changing unit tests to import modules from "src/" instead of importing the transpiled ones from "lib/" (and make sure they are being transpile on the fly by tests/babel-loader.js)
consider changing the way we are re-exporting the adb helper module (e.g. change it to export the adb transpiled module directly from the package.json)
fix tests related to importing web-ext as a library
investigate how the tests importing web-ext as a library are able to pass as is (because it seems suspicious they pass without any change)
compare new npm package size (with babel per-file transpile) with previous one (with webpack bundled)

other notes

NOTE: the following jscodeshift transform script has been created and used to aid the conversion to ES modules that loads successfully while using the nodejs native ES module loader:

https://gist.github.com/rpl/7b50f9d2a347c068d1b1185a0d173b09

.babelrc

.eslintrc

bin/web-ext.js

package.json

tests/babel-loader.js

tests/unit/test-extension-runners/test.firefox-android.js

codecov · 2022-05-31T19:29:27Z

Codecov Report

Merging #2405 (d320ef7) into master (c7fb58f) will decrease coverage by 99.88%.
The diff coverage is n/a.

❗ Current head d320ef7 differs from pull request most recent head a37dd24. Consider uploading reports for the commit a37dd24 to get more accurate results

@@            Coverage Diff             @@
##           master   #2405       +/-   ##
==========================================
- Coverage   99.88%       0   -99.89%     
==========================================
  Files          32       0       -32     
  Lines        1701       0     -1701     
==========================================
- Hits         1699       0     -1699     
+ Misses          2       0        -2

Impacted Files	Coverage Δ
src/cmd/build.js
src/cmd/docs.js
src/cmd/index.js
src/cmd/lint.js
src/cmd/run.js
src/cmd/sign.js
src/config.js
src/extension-runners/chromium.js
src/extension-runners/firefox-android.js
src/extension-runners/firefox-desktop.js
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7fb58f...a37dd24. Read the comment docs.

.babelrc

.eslintrc

tests/unit/helpers.js

package.json

src/cmd/run.js

rpl · 2022-06-01T15:48:08Z

@willdurand This PR does now cover everything that we discussed earlier today, in particular:

I looked into the reasons why the tests covering importing web-ext as a library were not failing and fixed the tests to make sure they cover the package being tested and not the last version released on npm (which I'm pretty sure it was the reason why those two tests were not yet failing)
added explicit coverage for the new behavior expected when trying to use web-ext as a library from a CommonJS module (asserting that it throws the error that nodejs throws internally in that scenario, and that the commonjs module can still use dynamic import to use web-ext internally)
changed util.logger and util.adb into additional exports declared in the package.json, covered that expected behavior in the functional tests and ripped off the workaround we were previously using to export these utilities from the webpack bundle (and related unit tests)
removed all inline TODOs related to Use dynamic imports "await import" instead of require #1306 (given that it is also fixed in this PR).

Then I also compared the package size as agreed, it is slightly bigger but imho not that much to be concerned about the difference:

new package size 165 kB (unpacked 699 kB) vs old package size 103 kB (unpacked 442 kB).

This is now ready for another review pass from your perspective.

willdurand

r+wc, thanks

index.js

package.json

- Changes to npm scripts to work as ESM modules (and replaced webpack with babel-cli as build step) - Add small custom node module loader (tests/babel-loader.js) to transpile on the fly (used to make mocha able to load tests modules using flowtype syntaxes) - Re-export custom node module loader exports provided by testdouble / quibble to be able to mock ES modules in some tests (used as part of workaround for sinon.stub limitations on stubbing ES modules) - Added babel-plugin-transform-inline-environment-variables to interpolate environment variable during babel transpiling (replaces webpack interpolated WEBEXT_BUILD_ENV global) - converted build scripts and fake firefox and amo scripts to ES modules - fixed sign-addon import when it is loaded using the nodejs native ES module loader (caught by functional tests)

README.md

…s ES module

rpl · 2022-06-03T13:36:50Z

@willdurand Thanks a lot for all the support and help reviewing this PR ❤️
I'll be merging it in a minute.

fregante

Great to see this change!

fregante · 2022-06-09T17:00:01Z

bin/web-ext.js

+const absolutePackageDir = path.join(
+  path.dirname(fileURLToPath(import.meta.url)),
+  '..'
+);


I think that that the new way would be to use URL instead of path, something like fileURLToPath(new URL('..', import.meta.url)), and you'd have the full path.

rpl requested a review from willdurand May 31, 2022 08:44

rpl force-pushed the fix/rewrite-into-esm branch 3 times, most recently from 82d5d16 to f53fc94 Compare May 31, 2022 09:19

willdurand reviewed May 31, 2022

View reviewed changes

rpl force-pushed the fix/rewrite-into-esm branch 3 times, most recently from e299a16 to 4797737 Compare May 31, 2022 19:24

willdurand reviewed Jun 1, 2022

View reviewed changes

.babelrc Outdated Show resolved Hide resolved

.eslintrc Outdated Show resolved Hide resolved

tests/unit/helpers.js Outdated Show resolved Hide resolved

package.json Show resolved Hide resolved

rpl force-pushed the fix/rewrite-into-esm branch 3 times, most recently from 6830b98 to b983aa1 Compare June 1, 2022 11:26

rpl commented Jun 1, 2022

View reviewed changes

src/cmd/run.js Show resolved Hide resolved

rpl force-pushed the fix/rewrite-into-esm branch 3 times, most recently from 57878a3 to a5f9590 Compare June 1, 2022 15:31

rpl requested a review from willdurand June 1, 2022 15:48

willdurand approved these changes Jun 2, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

willdurand reviewed Jun 2, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

willdurand mentioned this pull request Jun 2, 2022

Convert library to ES modules mozilla/sign-addon#1077

Merged

rpl force-pushed the fix/rewrite-into-esm branch 2 times, most recently from 8fc3ffb to 9d86ed5 Compare June 3, 2022 12:52

rpl force-pushed the fix/rewrite-into-esm branch from 9d86ed5 to d320ef7 Compare June 3, 2022 13:12

willdurand reviewed Jun 3, 2022

View reviewed changes

README.md Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

rpl added 2 commits June 3, 2022 15:26

feat(docs): Update README.md to mention how to import as native nodej…

b873b35

…s ES module

chore(deps): Updated package-lock.json

a37dd24

rpl force-pushed the fix/rewrite-into-esm branch from d320ef7 to a37dd24 Compare June 3, 2022 13:27

rpl merged commit 30ed3cc into master Jun 3, 2022

rpl deleted the fix/rewrite-into-esm branch June 3, 2022 13:37

fregante reviewed Jun 9, 2022

View reviewed changes

xM8WVqaG mentioned this pull request Feb 5, 2023

web-ext-config ES Module Support #2604

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: convert web-ext package into a nodejs native ES module #2405

feat: convert web-ext package into a nodejs native ES module #2405

rpl commented May 31, 2022 •

edited

Loading

codecov bot commented May 31, 2022 •

edited

Loading

rpl commented Jun 1, 2022

willdurand left a comment

rpl commented Jun 3, 2022

fregante left a comment

fregante Jun 9, 2022

feat: convert web-ext package into a nodejs native ES module #2405

feat: convert web-ext package into a nodejs native ES module #2405

Conversation

rpl commented May 31, 2022 • edited Loading

package size comparison

TODO list

other notes

codecov bot commented May 31, 2022 • edited Loading

Codecov Report

rpl commented Jun 1, 2022

willdurand left a comment

Choose a reason for hiding this comment

rpl commented Jun 3, 2022

fregante left a comment

Choose a reason for hiding this comment

fregante Jun 9, 2022

Choose a reason for hiding this comment

rpl commented May 31, 2022 •

edited

Loading

codecov bot commented May 31, 2022 •

edited

Loading