-
-
Notifications
You must be signed in to change notification settings - Fork 189
Add package exports for browser and umd #146
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
Add package exports for browser and umd #146
Conversation
fe2c186
to
b10e8c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your reporting this. A few tweaks, and then I'd need someone to vouch that the browser issue is importing as expected.
@@ -2,11 +2,16 @@ | |||
"author": "Stefan Goessner", | |||
"name": "jsonpath-plus", | |||
"version": "5.0.2", | |||
"type": "commonjs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep this. It helps make explicit how the behavior works. See https://nodejs.org/api/packages.html for the standard that webpack and Rollup plugins are or ought to be (and likely will be if they aren't already) following.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep this. It helps make explicit how the behavior works. See https://nodejs.org/api/packages.html for the standard that webpack and Rollup plugins are or ought to be (and likely will be if they aren't already) following.
Sorry, but with "type": "commonjs"
babel/webpack tell me:
ERROR in ./node_modules/jsonpath-plus/dist/index-browser-esm.js 1061:0
Module parse failed: 'import' and 'export' may appear only with 'sourceType: module' (1061:0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I've added a comment at webpack/webpack#11597 (comment) , as it looks to me that bundlers should not be using type
for interpretation of non-Node fields. There is a workaround there that may work for now without our changing the code, as I really think we should be able to use ".js" with browser ESM while using type: commonjs
for Node. If we don't hear back in some time, let's file a new Webpack issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And when revisiting this, Node recommends using default
over browser
(that probably makes sense too since other environments might support our own "vm" but not Node's).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a workaround there that may work for now without our changing the code
If by that work-around you mean adding something like:
module: {
rules: [
{
test: /browser-esm\.js$/,
include: /node_modules\/jsonpath-plus/,
type: "javascript/auto"
},
…
]
}
to my webpack config, then: no, that doesn't work. Webpack 5 still loads the node version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and filed webpack/webpack#12598 as I do recall you had this before. Will see about a response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given our earlier discussion of this, I wanted to let you know (and anyone watching this) that I am planning to release as a true ESM module by using type: 'module'
(otherwise our default ".js" is not understood in some environments as being a genuine module, e.g., Mocha 9 fails completely, and I think it has only been working in modules up to now because ESM can import CJS as we provide).
Would you mind testing out the esm
branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brettz9 Where do I find this esm
branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brettz9 Appears to work fine; apologies for the delay.
b10e8c9
to
f6d5891
Compare
f6d5891
to
6f2493c
Compare
PR description
Add package exports for browser and umd
Fixes #145
Checklist
npm test
, ensuring linting passes