-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fix build #38
Conversation
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
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. The removing of the bundling of Also, I update the target to es6. Any objections? @jrieken, that also impacts the commonjs/umd code. |
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 |
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. |
Made obsolete by #39 |
Previously only a umd main entrypoint. There was also the non-standard
module
field inpackage.json
, which causes issues with various tools. Now the standardpackage.json
fieldexports
is used to support both native ESM and CJS.Additionally, the webpack build has been replaced with
tsc
. Instead of bundlingpath-browserify
, an import condition is used to select eitherpath
orpath-browserify
. This means that browser users usepath-browserify
, regardless of whether they use ESM or CJS, whereas others usepath
. Also this means users can deduplicatepath-browserify
if they use it elsewhere in their code base.The
browser
condition is could be tested using:However, this is currently broken due to browserify/path-browserify#33.
Closes #25
Closes #33
Closes #37