Skip to content
This repository has been archived by the owner on Jan 9, 2024. It is now read-only.

Convert library to ES modules #1077

Merged
merged 1 commit into from
Jun 3, 2022
Merged

Convert library to ES modules #1077

merged 1 commit into from
Jun 3, 2022

Conversation

willdurand
Copy link
Member

@willdurand willdurand commented Jun 2, 2022

Fixes #1078


This PR converts the library to ES modules and this is similar to mozilla/web-ext#2405.

Summary

  • Convert the entire library to ES modules
  • No more webpack, it is using babel like web-ext
  • ESM library that exposes signAddon and signAddonAndExit with the option of dynamically importing the library in commonjs (await import())

Public API

I verified the public API by npm pack'ing this work and npm i /path/to/sign-addon-4.1.0.tgz in a temporary directory. Then I created two scripts, one for commonjs and one for esmodules. The APIs are the same except that commonjs has a default, which I think is expected.

❯ cat test.js
  (async () => {
      let signAddon = await import('sign-addon');

      console.log(signAddon)
  })();

❯ node test.js
[Module: null prototype] {
  default: [Module: null prototype] {
    signAddon: [AsyncFunction: signAddon],
    signAddonAndExit: [AsyncFunction: signAddonAndExit]
  },
  signAddon: [AsyncFunction: signAddon],
  signAddonAndExit: [AsyncFunction: signAddonAndExit]
}

and:

❯ cat test.mjs
  import signAddon from 'sign-addon';

  console.log(signAddon)

❯ node test.mjs
[Module: null prototype] {
  signAddon: [AsyncFunction: signAddon],
  signAddonAndExit: [AsyncFunction: signAddonAndExit]
}

@willdurand willdurand force-pushed the esmification branch 5 times, most recently from 9ab3d46 to c28d4aa Compare June 2, 2022 11:05
@willdurand willdurand requested a review from rpl June 2, 2022 11:10
@willdurand willdurand marked this pull request as ready for review June 2, 2022 11:10
Comment on lines 72 to 74
// @ts-ignore - TypeScript refuses to change the type of the mock...
signingCall = jest.fn().mockImplementation(
/**
* @param {SignParams} params
*/
// eslint-disable-next-line no-unused-vars
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 have no idea what has changed here tbh :/

Copy link
Member

Choose a reason for hiding this comment

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

not sure what made tsc to start to complain, but it seems that it may be able to stop complaining with a bit of help (but I haven't used typescript as much as flowtype and so I don't exclude I may have tricked tsc to stop complaining in an incorrect way)

The following tweaked version passed npm run typecheck (and also npm run lint and npm run test) for me locally and so may consider if we prefer using this approach as an alternative to the @ts-ignore (not a big deal though):

diff --git a/tests/index.spec.js b/tests/index.spec.js
index 1500c48..a673759 100644
--- a/tests/index.spec.js
+++ b/tests/index.spec.js
@@ -69,17 +69,20 @@ describe(fileURLToPath(import.meta.url), () => {
         // @ts-ignore
         this.debug = jest.fn();
 
-        // @ts-ignore - TypeScript refuses to change the type of the mock...
-        signingCall = jest.fn().mockImplementation(
-          // eslint-disable-next-line no-unused-vars
-          (params) =>
-            new Promise((resolve) => {
-              if (options.errorToThrow) {
-                throw options.errorToThrow;
-              }
-              resolve(options.result);
-            }),
-        );
+        /** @type {(params: SignParams) => Promise<SignResult>} */
+        // eslint-disable-next-line no-unused-vars
+        const mockedSigningCall = (params) =>
+          new Promise((resolve) => {
+            if (options.errorToThrow) {
+              throw options.errorToThrow;
+            }
+            resolve(options.result);
+          });
+
+        /**
+         * @type {jest.Mock<(params: SignParams) => Promise<SignResult>>}
+         */
+        signingCall = jest.fn(this.sign).mockImplementation(mockedSigningCall);

@willdurand willdurand force-pushed the esmification branch 3 times, most recently from 1909603 to 3d984eb Compare June 3, 2022 11:32
Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

@willdurand this looks great, follows a couple of comments, but they seems mainly nice to have and so I'm approving this version along with submitting those.

I'm happy to give this another look if we decide to integrate some more tweaks from the review comments below.

.gitignore Show resolved Hide resolved
.eslintrc Show resolved Hide resolved
tests/functional/imports.spec.js Outdated Show resolved Hide resolved
Comment on lines 72 to 74
// @ts-ignore - TypeScript refuses to change the type of the mock...
signingCall = jest.fn().mockImplementation(
/**
* @param {SignParams} params
*/
// eslint-disable-next-line no-unused-vars
Copy link
Member

Choose a reason for hiding this comment

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

not sure what made tsc to start to complain, but it seems that it may be able to stop complaining with a bit of help (but I haven't used typescript as much as flowtype and so I don't exclude I may have tricked tsc to stop complaining in an incorrect way)

The following tweaked version passed npm run typecheck (and also npm run lint and npm run test) for me locally and so may consider if we prefer using this approach as an alternative to the @ts-ignore (not a big deal though):

diff --git a/tests/index.spec.js b/tests/index.spec.js
index 1500c48..a673759 100644
--- a/tests/index.spec.js
+++ b/tests/index.spec.js
@@ -69,17 +69,20 @@ describe(fileURLToPath(import.meta.url), () => {
         // @ts-ignore
         this.debug = jest.fn();
 
-        // @ts-ignore - TypeScript refuses to change the type of the mock...
-        signingCall = jest.fn().mockImplementation(
-          // eslint-disable-next-line no-unused-vars
-          (params) =>
-            new Promise((resolve) => {
-              if (options.errorToThrow) {
-                throw options.errorToThrow;
-              }
-              resolve(options.result);
-            }),
-        );
+        /** @type {(params: SignParams) => Promise<SignResult>} */
+        // eslint-disable-next-line no-unused-vars
+        const mockedSigningCall = (params) =>
+          new Promise((resolve) => {
+            if (options.errorToThrow) {
+              throw options.errorToThrow;
+            }
+            resolve(options.result);
+          });
+
+        /**
+         * @type {jest.Mock<(params: SignParams) => Promise<SignResult>>}
+         */
+        signingCall = jest.fn(this.sign).mockImplementation(mockedSigningCall);

@willdurand willdurand merged commit 2aec92a into master Jun 3, 2022
@willdurand willdurand deleted the esmification branch June 3, 2022 15:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to ES modules
2 participants