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

Add package.json exports field #773

Merged
merged 3 commits into from
Mar 14, 2021
Merged

Conversation

manzt
Copy link
Contributor

@manzt manzt commented Mar 3, 2021

Adds the new "exports" field in package.json to define conditional exports for mustache.js (ESM & CJS). This field is used both by Node and bundlers to resolve dependencies. Since there is no "module" entrypoint for this package, some bundlers (rollup + @rollup/plugin-node-resolve) cannot resolve the module export. This is a step towards making both modules discoverable.

Example

// package.json
{
  "main": "index.js",
  "type": "module",
  "scripts": {
    "build": "rollup -c"
  },
  "dependencies": {
    "@rollup/plugin-node-resolve": "^11.2.0",
    "mustache": "^4.1.0",
    "rollup": "^2.40.0"
  }
}
import Mustache from 'mustache';

const view = {
  title: "Joe",
  calc: function () {
    return 2 + 4;
  }
};

const output = Mustache.render("{{title}} spends {{calc}}", view);
console.log(output)
// rollup.config.js
import resolve from '@rollup/plugin-node-resolve';

export default {
  input: 'index.js',
  output: { file: 'bundle.js', format: 'esm' },
  plugins: [ resolve() ]
}
❯ npm run build

> rollup -c


index.js → bundle.js...
(!) `this` has been rewritten to `undefined`
https://rollupjs.org/guide/en/#error-this-is-undefined
node_modules/mustache/mustache.js
4:   typeof define === 'function' && define.amd ? define(factory) :
5:   (global = global || self, global.Mustache = factory());
6: }(this, (function () { 'use strict';
     ^
7:
8:   /*!
[!] Error: 'default' is not exported by node_modules/mustache/mustache.js, imported by index.js
https://rollupjs.org/guide/en/#error-name-is-not-exported-by-module
index.js (1:7)
1: import Mustache from 'mustache';
          ^
2:
3: const view = {
Error: 'default' is not exported by node_modules/mustache/mustache.js, imported by index.js
    at error (/Users/fm813/github/manzt/reference-spec-reader/node_modules/rollup/dist/shared/rollup.js:5279:30)
    at Module.error (/Users/fm813/github/manzt/reference-spec-reader/node_modules/rollup/dist/shared/rollup.js:9996:16)
    at Module.traceVariable (/Users/fm813/github/manzt/reference-spec-reader/node_modules/rollup/dist/shared/rollup.js:10389:29)
    at ModuleScope.findVariable (/Users/fm813/github/manzt/reference-spec-reader/node_modules/rollup/dist/shared/rollup.js:8847:39)
    at MemberExpression.bind (/Users/fm813/github/manzt/reference-spec-reader/node_modules/rollup/dist/shared/rollup.js:6535:49)
    at CallExpression.bind (/Users/fm813/github/manzt/reference-spec-reader/node_modules/rollup/dist/shared/rollup.js:2728:23)
    at CallExpression.bind (/Users/fm813/github/manzt/reference-spec-reader/node_modules/rollup/dist/shared/rollup.js:6730:15)
    at VariableDeclarator.bind (/Users/fm813/github/manzt/reference-spec-reader/node_modules/rollup/dist/shared/rollup.js:2728:23)
    at VariableDeclaration.bind (/Users/fm813/github/manzt/reference-spec-reader/node_modules/rollup/dist/shared/rollup.js:2724:31)
    at Program.bind (/Users/fm813/github/manzt/reference-spec-reader/node_modules/rollup/dist/shared/rollup.js:2724:31)

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @ build: `rollup -c`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the @ build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/fm813/.npm/_logs/2021-03-03T20_51_56_547Z-debug.log

@phillipj
Copy link
Collaborator

phillipj commented Mar 3, 2021

Thanks @manzt!

Getting an exports field in, is indeed interesting.

Heads up about a few changes planned to be merge from #772, which touches upon the GitHub usage tests at least..

@manzt
Copy link
Contributor Author

manzt commented Mar 3, 2021

No problem. Let me know if there is something else I can do to help. I notice skypack is mentioned in #772; FYI importing mustache currently via skypack returns a (skypack generated) ESM wrapping around the CJS module because skypack can't find ESM source (it uses package exports).

<script type="module">
  import mustache from 'https://cdn.skypack.dev/mustache'; // skypack resolves mustache.js -> transpiles to ESM -> sends back
</script>

skypack cdn source: https://cdn.skypack.dev/-/[email protected]/dist=es2020,mode=imports/optimized/mustache.js

EDIT: Also, I totally sympathize with trying to target multiple environments. This project does so much more than most to make usage seamless. Thanks so much, it's a great library.

@phillipj
Copy link
Collaborator

phillipj commented Mar 5, 2021

Myeah, I also noticed the skypack story isn't ideal ATM. Guess it works, but let's try and do better 😄

Now that #772 has been merged, I'll try to wrap my head around the changes in this PR. I'm also pretty sure I've seen mentioned that adding an exports field should be seen as a semver major change. Does that ring a bell to you? I'll try and track down where I got those rumours from..

This project does so much more than most to make usage seamless. Thanks so much, it's a great library.

Really appreciate hearing that, thanks a bunch!

@manzt
Copy link
Contributor Author

manzt commented Mar 9, 2021

I'm also pretty sure I've seen mentioned that adding an exports field should be seen as a semver major change. Does that ring a bell to you?

I had not seen this before, but I'm glad that you had! Feeling a bit ignorant for suggesting such a major change when this is clearly stated in the Node.js docs: https://nodejs.org/api/packages.html#packages_package_entry_points

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.

This change is apparently to allow library authors to encapsulate their packages, allowing fine-grain control over public interfaces for packages. With that said, I noticed the previous ESM imports in node were mustache/mustache.mjs so I added the export "./*": "./*" naively to keep previous tests passing. This is also mentioned in the docs:

As a last resort, package encapsulation can be disabled entirely by creating an export for the root of the package "./*": "./*". This exposes every file in the package at the cost of disabling the encapsulation and potential tooling benefits this provides.

TL;DR I think the "./*": "./*" export negates requiring a semver major change, but it's recommended as a "last resort" because you lose the encapsulation that export maps offer to library authors. If the latter is important, it probably makes sense to wait for a semver major change.

@phillipj
Copy link
Collaborator

phillipj commented Mar 14, 2021

Thanks @manzt, great research 👍🏻

With that said, I noticed the previous ESM imports in node were mustache/mustache.mjs so I added the export "./*": "./*" naively to keep previous tests passing

Sounds good to me, as that also ensures we keep backwards compatibility with what we've been telling ESM users to do for well over a year.

I think the "./*": "./*" export negates requiring a semver major change, but it's recommended as a "last resort" because you lose the encapsulation that export maps offer to library authors.

I don't consider encapsulation via the exports field important for us to be honest. The majority of what the npm package consists of, is only one .js file after all.

Looking at what our resulting package tarball consists of:

npm notice === Tarball Contents === 
npm notice 1.2kB  LICENSE                           
npm notice 3.5kB  bin/mustache                      
npm notice 25.2kB mustache.js                       
npm notice 11.8kB mustache.min.js                   
npm notice 2.0kB  package.json                      
npm notice 22.1kB CHANGELOG.md                      
npm notice 16.8kB README.md                         
npm notice 23.5kB mustache.mjs                      
npm notice 58B    wrappers/dojo/mustache.js.post    
npm notice 359B   wrappers/jquery/mustache.js.post  
npm notice 128B   wrappers/mootools/mustache.js.post
npm notice 184B   wrappers/qooxdoo/mustache.js.post 
npm notice 45B    wrappers/yui3/mustache.js.post    
npm notice 192B   wrappers/dojo/mustache.js.pre     
npm notice 141B   wrappers/jquery/mustache.js.pre   
npm notice 14B    wrappers/mootools/mustache.js.pre 
npm notice 6.4kB  wrappers/qooxdoo/mustache.js.pre  
npm notice 34B    wrappers/yui3/mustache.js.pre     

..I don't see any of those files as "private" implementation details we'd want to avoid exposing to the outside. That's basically what we've been using the files field for already:

mustache.js/package.json

Lines 21 to 26 in 3e29d67

"files": [
"bin/",
"mustache.mjs",
"mustache.min.js",
"wrappers/"
],

Bottom line: I think this is good to go.

@phillipj phillipj changed the title Add package exports Add package.json exports field Mar 14, 2021
@phillipj phillipj merged commit ea3adcf into janl:master Mar 14, 2021
@phillipj
Copy link
Collaborator

Thanks a lot for making this happen, I'm sure it'll make many devs out there happy 💯

I published 4.2.0-beta.0 to npm if anyone would like to give this a shot ASAP:

$ npm install mustache@beta

@manzt manzt deleted the add-package-exports branch March 14, 2021 15:56
@phillipj
Copy link
Collaborator

This has now been elevated from beta -> latest and pushed to npm as v4.2.0.

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.

2 participants