Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

browser package should be a module #237

Merged
merged 2 commits into from
Aug 5, 2022
Merged

Conversation

bomgar
Copy link
Contributor

@bomgar bomgar commented Aug 3, 2022

fixes #236

@bomgar
Copy link
Contributor Author

bomgar commented Aug 3, 2022

To be honest I don't really know what I'm doing here. I just fixed the errors I got.

Copy link
Owner

@MasterKale MasterKale left a comment

Choose a reason for hiding this comment

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

@bomgar Thank you for the submission. Just a few small tweaks and I'd say this is ready to go. Thank you for including the changes to the test configs.

packages/browser/jest.config.js Show resolved Hide resolved
packages/browser/jest-environment.js Show resolved Hide resolved
packages/browser/jest-environment.js Show resolved Hide resolved
@bomgar
Copy link
Contributor Author

bomgar commented Aug 3, 2022

@MasterKale i did change the file to mjs but now the server import doesn't work anymore.

@bomgar
Copy link
Contributor Author

bomgar commented Aug 3, 2022

@MasterKale I changed the server to type module, too. Now the build is green. Is this ok?

@MasterKale
Copy link
Owner

@MasterKale I changed the server to type module, too. Now the build is green. Is this ok?

I did a before and after build of server after adding "type": "module" to its package.json, and the diff says nothing changed so I thought it'd be fine to add that:

Screen Shot 2022-08-03 at 5 13 22 PM

However it appears adding that breaks projects that use require(), as I observed in the example project when I tried using it:

/Users/matt/Developer/simplewebauthn/example/index.ts:43
const app = express();
                 ^
example/node_modules/@cspotcode/source-map-support/source-map-support.js:590
Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/matt/Developer/simplewebauthn/packages/server/dist/index.js from /Users/matt/Developer/simplewebauthn/example/index.ts not supported.
index.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
Instead rename index.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in /Users/matt/Developer/simplewebauthn/packages/server/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).

I wonder now if it's worth trying to preserve the deduplication of the Jest config...

Tell you what, can you actually get rid of jest.config.mjs, and just copy its three config lines into the browser and server jest configs? Then forget about adding "type": "module" to server's package.json, that's not something this project can afford to break right now.

@P4sca1
Copy link
Contributor

P4sca1 commented Aug 4, 2022

This PR updates the server package to "type": "module", which is wrong, as it is currently compiled into a CJS module.

It is a good idea, to convert the package to pure ESM. Therefore the tsconfig.json file needs to be updated, so that valid ESM is produced. This would be a breaking change, since the module could no longer be require()'d.
This gist provides very helpful information around this topic: https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

An alternative approach would be to produce both an ESM bundle and a CJS bundle and use conditional exports. esbuild could be used instead of rollup to do the job. I have a similar build pipeline here for reference: https://github.com/P4sca1/cron-schedule/tree/v3.0.6.

Personally, I would go with the ESM only approach, as it is easier to setup and maintain. This is also what other libraries did in the past - it is time to move away from CJS to ESM. This should then be a major release, which should maybe include the gist I referenced earlier.

@MasterKale
Copy link
Owner

Server currently works fine in both ESM and CJS codebases, so I don't immediately see the need to make any changes to it. That's why I mention forgetting about adding "type": "module" to its package.json.

We can explore the possibility of refactoring server's build but it's definitely outside the scope of this PR.

@bomgar
Copy link
Contributor Author

bomgar commented Aug 5, 2022

@MasterKale I made the adjustments.

@MasterKale MasterKale added this to the 5.4.1 milestone Aug 5, 2022
@MasterKale MasterKale merged commit e83e8a9 into MasterKale:master Aug 5, 2022
@MasterKale
Copy link
Owner

Thank you for your contribution, @bomgar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:browser @simplewebauthn/browser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vitejs complains about the browser package
3 participants