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

Added "exports"-field to package.json so import/require loads the appropriate file without any additional user-input #1725

Closed
wants to merge 5 commits into from

Conversation

KilianKilmister
Copy link

Marked version: @latest

Markdown flavor: n/a

Description

I did a superficial test to ensure the paths are correct.
VSCode debugger showed that import marked from 'marked' will only load lib/marked.esm.mjs

citing my comment

cjs-require/esm-import can be automatically handled by specifying an "exports" field in the package.json.
This way node will load the appropriate files without the user having to worry about in-module paths.

Short explanation

{
  "exports": {
    [exportPath]: {
      "import": "./path/to/file.mjs",
      "require": "./path/to/file.cjs"
    }
  }
}

So for marked it would be:

{
  "exports": {
    ".": {
      "import": "./lib/marked.esm.mjs", // `.mjs`-extention is needed
      "require": "./src/marked.js"
    }
  }
}

I'll file a PR for that shortly

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
  • no tests required for this PR.
  • If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

kilian added 3 commits July 9, 2020 13:13
@vercel
Copy link

vercel bot commented Jul 9, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/markedjs/markedjs/j688ptxsg
✅ Preview: https://markedjs-git-fork-kiliankilmister-master.markedjs.vercel.app

@KilianKilmister KilianKilmister changed the title Added "exports"-field to package.json so import/require loads the appropriate file without requiering additional user-input Added "exports"-field to package.json so import/require loads the appropriate file without any additional user-input Jul 9, 2020
Signed-off-by: kilian <kilian@catsoft.com>
@KilianKilmister
Copy link
Author

#1692

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@UziTech
Copy link
Member

UziTech commented Jul 9, 2020

According to Node:

Warning: Introducing the "exports" field prevents consumers of a package from using any entry points that are not defined, including the package.json (e.g. require('your-package/package.json'). This will likely be a breaking change.

So we will need to do a major version bump for this.

Comment on lines +43 to +44
"@types/node": "^14.0.9",
"babel-eslint": "^10.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

why are these extra dependencies on here?

Copy link
Author

Choose a reason for hiding this comment

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

nodejs typings and a linter that doesn't croak on encountering stage3 ES-features. this was a rather hasty PR so i forgot to remove my devDeps.
There's little use for the linter if you don't use features released in after 2018.
node typings are always usefull. i don't think you use any builtins in the main code, but the tests do. i honestly recommend adding them to any project.

@UziTech
Copy link
Member

UziTech commented Jul 9, 2020

I also don't believe we should implement this until es modules are not experimental as the interface could still change.

It seems this is only supported in node v14+ so we should probably wait until v14 becomes LTS.

Copy link
Author

@KilianKilmister KilianKilmister left a comment

Choose a reason for hiding this comment

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

According to Node:

Warning: Introducing the "exports" field prevents consumers of a package from using any entry points that are not defined, including the package.json (e.g. require('your-package/package.json'). This will likely be a breaking change.

So we will need to do a major version bump for this.

Is importing a package.json a "common" occurance? Just screams bad practice to me. But if that's something that people do, then it's certainly a point to consider.

LTS for node v14 is shockingly close already i just realised (scheduled for late october as always). I think this can wait till then

Comment on lines +43 to +44
"@types/node": "^14.0.9",
"babel-eslint": "^10.1.0",
Copy link
Author

Choose a reason for hiding this comment

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

nodejs typings and a linter that doesn't croak on encountering stage3 ES-features. this was a rather hasty PR so i forgot to remove my devDeps.
There's little use for the linter if you don't use features released in after 2018.
node typings are always usefull. i don't think you use any builtins in the main code, but the tests do. i honestly recommend adding them to any project.

@UziTech
Copy link
Member

UziTech commented Jul 9, 2020

Is importing a package.json a "common" occurance?

Not package.json, but I know there are some dependents that import just certain files like require('marked/lib/marked.js') or require('marked/src/Renderer.js'). We would need to define all valid entry points in the "exports" field not just ".".

@KilianKilmister
Copy link
Author

Is importing a package.json a "common" occurance?

Not package.json, but I know there are some dependents that import just certain files like require('marked/lib/marked.js') or require('marked/src/Renderer.js'). We would need to define all valid entry points in the "exports" field not just ".".

This would be the files in src and lib, right? We could list those folders and expose them as a whole

@evanplaice
Copy link

It seems this is only supported in node v14+ so we should probably wait until v14 becomes LTS.

This isn't true. ESM was introduced in Node 13.x and back ported to 12.x LTS. With 10.x being EOL'd there is no longer an officially supported version of Node that doesn't work with modules.

For the issue of direct path imports being unavailable after pkg.exports is introduced. Instead of defining an export for every file you can provide a facade instead that re-exports all of the modules. Like this (https://github.com/vanillaes/absurdum/blob/main/src/arrays/index.js) but using default exports because none of the existing components are written in ESM.

If you don't want to support 2 copies (ie CJS and ESM), you can always use RollupJS or ESBuild to convert from ESM to CJS. ESM -> CJS conversion is a LOT cleaner than CJS -> ESM.

@UziTech
Copy link
Member

UziTech commented May 11, 2021

If this PR was rebased I would be fine with including the exports field now 👍

@UziTech UziTech mentioned this pull request Aug 3, 2021
5 tasks
".": {
"import": "./lib/marked.esm.mjs",
"require": "./src/marked.js"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you should also export package.json as some bundlers will want to read it

Suggested change
}
},
"./package.json": "./package.json"

@@ -0,0 +1,2206 @@
/**
Copy link
Contributor

@benmccann benmccann Aug 3, 2021

Choose a reason for hiding this comment

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

If this file is going to be added then the old lib/marked.esm.js should be removed, but it shouldn't be necessary to name it with an .mjs extension

@UziTech
Copy link
Member

UziTech commented Oct 24, 2021

closing this in favor of #2227

@UziTech UziTech closed this Oct 24, 2021
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.

None yet

4 participants