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

Fix build #38

Closed
wants to merge 5 commits into from
Closed

Fix build #38

wants to merge 5 commits into from

Conversation

remcohaszing
Copy link

Previously only a umd main entrypoint. There was also the non-standard module field in package.json, which causes issues with various tools. Now the standard package.json field exports is used to support both native ESM and CJS.

Additionally, the webpack build has been replaced with tsc. Instead of bundling path-browserify, an import condition is used to select either path or path-browserify. This means that browser users use path-browserify, regardless of whether they use ESM or CJS, whereas others use path. Also this means users can deduplicate path-browserify if they use it elsewhere in their code base.

The browser condition is could be tested using:

node --conditions browser node_modules/.bin/mocha

However, this is currently broken due to browserify/path-browserify#33.

Closes #25
Closes #33
Closes #37

Previously only a umd main entrypoint. There was also the non-standard
`module` field in `package.json`, which causes issues with various
tools. Now the standard `package.json` field `exports` is used to
support both native ESM and CJS.

Additionally, the webpack build has been replaced with `tsc`. Instead of
bundling `path-browserify`, an import condition is used to select either
`path` or `path-browserify`. This means that browser users use
`path-browserify`, regardless of whether they use ESM or CJS, whereas
others use `path`. Also this means users can deduplicate
`path-browserify` if they use it elsewhere in their code base.

Closes microsoft#25
Closes microsoft#33
Closes microsoft#37
@aeschli
Copy link
Contributor

aeschli commented Sep 13, 2023

Thanks for the nice work!

I pushed a small fix for a problem I found when consuming the ESM bits from our webpack scripts. Imports can be node modules, but webpack looks for them on disk. path beeing a built-in node module wasn't found.

The removing of the bundling of path has advantages and disadvantages. Before vscode-uri was all selfcontained and easy to consume. I hope that all packages can handle the imports syntax.
So maybe it would be on the safer sideto keep the code inlined and just fix the exports.

Also, I update the target to es6. Any objections? @jrieken, that also impacts the commonjs/umd code.

@aeschli
Copy link
Contributor

aeschli commented Sep 18, 2023

I discussed this with @jrieken. We want to keep the path code inlined and vscode-uri selfcontained

Note that both UMD and ESM variants bundle the path code. They have the identical functionality. It's not that ESM is 'for browsers'.

Still, the changes to add exports and to add esm/package.json are good and valuable.

@remcohaszing
Copy link
Author

Do you want the types to be technically correct or is it sufficient if it works, but is technically not 100% correct?

Dual publishing requires to publish types for both CJS and ESM, but when only using named exports, the package is also usable when only publishing the CJS types.

If it doesn’t have to be 100% correct, #25 can be used as-is. If it does need to be correct, I will need to look into another build tool. tsup should probably work.

@aeschli aeschli mentioned this pull request Oct 3, 2023
@aeschli
Copy link
Contributor

aeschli commented Oct 5, 2023

Made obsolete by #39

@aeschli aeschli closed this Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants