Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 5, 2021
Merged

Add package exports for browser and umd #146

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@
"author": "Stefan Goessner",
"name": "jsonpath-plus",
"version": "5.0.2",
"type": "commonjs",
Copy link
Collaborator

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.

Copy link
Author

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)

Copy link
Collaborator

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.

Copy link
Collaborator

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).

Copy link
Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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?

Copy link
Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, sorry, https://github.com/brettz9/JSONPath/tree/esm , i.e., cloning https://github.com/brettz9/JSONPath#esm

Copy link
Author

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.

"main": "dist/index-node-cjs.js",
"exports": {
"import": "./dist/index-node-esm.mjs",
"require": "./dist/index-node-cjs.js"
".": {
"browser": "./dist/index-browser-esm.js",
"umd": "./dist/index-browser-umd.js",
"import": "./dist/index-node-esm.mjs",
"require": "./dist/index-node-cjs.js",
"default": "./dist/index-browser-esm.js"
}
},
"module": "dist/index-node-esm.mjs",
"browser": "dist/index-browser-esm.js",
Expand Down