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

Update package.json #231

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

frank-dspeed
Copy link

Add a extra package.json to the es2015 build that sets the nodejs esm flag if used inside nodejs

Add a extra package.json to the es2015 build that sets the nodejs esm flag if used inside nodejs
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling b5fdc85 on frank-dspeed:patch-1 into d8bc41f on pillarjs:master.

@blakeembrey
Copy link
Member

Are you able to add a test that ensures this works? I'm not currently qualified to judge this addition.

@frank-dspeed
Copy link
Author

@blakeembrey what exactly should i test for?

  • That the file gets created with right content?
  • That nodejs and other is tools are able to load the esm version?

we create simply a extra package.json inside the dist.es2015 folder with content type: "module" without that package.json this will get treated as commonjs and so it will fail loading. there are other ways like renaming it to .mjs but we do not want to do that as this leads to other incompatiblitys. That is the best fix at the moment you can create that package.json with a other method if you like but i think this one is quick and dirty.

@blakeembrey
Copy link
Member

blakeembrey commented Oct 11, 2020

That the file gets created with right content?

Less important since I assume this is solved with:

That nodejs and other is tools are able to load the esm version?

How would someone require this new package that you're adding? Can you add documentation about how it works or how someone might use it to the README? Is it automatically supported by node.js (my understanding was no)?

Edit: wouldn't it be preferable to follow https://nodejs.org/api/packages.html#packages_conditional_exports instead?

@frank-dspeed
Copy link
Author

frank-dspeed commented Oct 12, 2020

@blakeembrey even if you follow that it is needed without that file a import from nodejs is not possible

import pathtoregexp from 'path-to-regexp/es2015/*.js'

will throw that it gets a esm package but the package is marked as CJS :)

the *.js extension with esm content only works when the package.json includes type: module it will recursiv look for the next package.json to verify that.

or you rename it to .mjs but that has its own problems :)

@frenzzy
Copy link
Contributor

frenzzy commented Oct 12, 2020

I think during this PR we should also add type and exports fields to the main package.json so all the possible cases will work:

// package.json
{
  ...
  "type": "commonjs", // explicitly mark all ".js" files in the folder as CJS
  "main": "dist/index.js", // used via "require" in legacy node.js
  "typings": "dist/index.d.ts",
  "module": "dist.es2015/index.js", // used by bundlers (webpack, parcel, rollup, etc.)
  "exports": {
    "require": "dist/index.js", // used via "require" in modern node.js
    "default": "dist.es2015/index.js" // used via "import" in modern node.js
  },
  ...
}

// dist.es2015/package.json
{
  "type": "module" // explicitly mark all ".js" files in the folder as ESM
}

i.e. users will be able to use both

const { pathToRegexp, match, parse, compile } = require("path-to-regexp");

and

import { pathToRegexp, match, parse, compile } from "path-to-regexp";

in Node.js depending on their own preferences.

@frank-dspeed
Copy link
Author

@frenzzy type fild for cjs is not needed it is already the default if you use a node version that supports it exports should not be a part of this pr this pr simply should allow that you can import the file if you want to import it thats it.

i runnend into that and did not want to make a fork for that while i now did a fork of this anyway.

i only wanted to backport that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants