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

fix: main field in package.json should correspond to cjs artifacts #5756

Open
wants to merge 1 commit into
base: v1.x
Choose a base branch
from

Conversation

thebanjomatic
Copy link

@thebanjomatic thebanjomatic commented Jun 29, 2023

When #4787 was implemented, the project was switched to "type": "module" and "./index.js" became an esm file instead of commonjs, however, the "main" entry in package.json still points to "index.js". As a result, consumers using this field may get unexpected behavior since the main field is supposed to be commonjs if the entry is provided.

Many consumers won't run into this as a practical problem (for example when just doing const axios = require('axios').default from inside of a cjs file in node) because the "exports" map takes precedence over the main/module fields, but tools that don't parse the exports map when resolving still run into problems here. As an example, I encountered this problem when using the resolve package as part of a workflow for a jest config.

The fix for this is to just point the "main" entry-point to the commonjs artifacts located at "./dist/node/axios.cjs".

I also added a module entrypoint to improve compatibility for the cases where the export map is not used (webpack 4 for example) since that would likely be reading the cjs "main" entrypoint now that main has switched back to cjs.

When axios#4787 was implemented, the project was switched to `"type": "module"` and "./index.js" became an esm file instead of commonjs, however, the "main" entry in package.json still points to "index.js". As a result, consumers using this field may get unexpected behavior since the main field is supposed to be commonjs if the entry is provided.

Many consumers won't run into this as a practical problem (for example when just doing `const axios = require('axios').default` from inside of a cjs file in node) because the "exports" map takes precedence over the main/module fields, but tools that don't parse the object map when resolving still run into problems here.

The fix for this is to just point the "main" entry-point to the commonjs artifacts located at "./dist/node/index.cjs".

I also added a module entrypoint to improve compatability for the cases where the export map is not used (webpack 4 for example) since that would likely be reading the cjs "main" entrypoint now that main has switched back to cjs.
@thebanjomatic
Copy link
Author

This PR has been sitting for a few months but the underlying issue has been causing me all sorts of headaches as we need to keep adding the following to our jest.config.js to work around these issues which has been confusing for our end users (since they aren't importing axios directly and its a transitive dependency):

  moduleNameMapper: {
    '^axios$': require.resolve('axios'),
  },

@DigitalBrainJS I don't know who the current project owners are, but is this something you are able to review and release?

@DigitalBrainJS
Copy link
Collaborator

the project was switched to "type": "module" and "./index.js" became an esm file instead of commonjs, however, the "main" entry in package.json still points to "index.js".

Why do you think this should not be so? Axios is ESM module and according to the documentation, the main field must point to an ESM module if the package has "type: module". "Module" field is not in npm spec and it is just a proprietary field for some bundlers. According to the npm documentation, backward compatibility with commonjs for an ESM package should be handled by conditional export.
Axios should follow the official specification, and not adapt to special cases arising from bugs, obsolescence, and inconsistencies of third-party libraries. If a third-party library has a bug or lack of support for functionality, then it must be resolved on its part.
In addition, node v12.0-12.7 does not support conditional export so the main field will be used.

@thebanjomatic
Copy link
Author

thebanjomatic commented Oct 6, 2023

Why do you think this should not be so? Axios is ESM module and according to the documentation, the main field must point to an ESM module if the package has "type: module".

I haven't found that in the node documentation specifically that implies the main field must be the es-module the package type is module. I could only find documentation that suggested that it could point to cjs or esm (in which case the package would not be consumable from a require() expression because you can't require esm files.

For example, there is the following quote:

All currently supported versions of Node.js and modern build tools support the "exports" field. For projects using an older version of Node.js or a related build tool, compatibility can be achieved by including the "main" field alongside "exports" pointing to the same module:

{
 "main": "./index.js",
 "exports": "./index.js"
} 

While that says the can point to the same module, their example doesn't use conditional exports or specify "type": "module", so I think the documentation is unclear. That certainly would be compatible with old versions of node assuming index.js is commonjs (which it would be given the extension and lack of type entry), but if that was an es-module it would fail to be require in old versions of node, and you would only support import in node 12.0-7. Since you can both import and require cjs entries from cjs and esm, it follows that setting the main entrypoint to point to the commonjs entry would be the most compatible way to specify the main entry for cases where exports is not used.

https://nodejs.org/api/packages.html
https://nodejs.org/api/esm.html#modules-ecmascript-modules

Axios should follow the official specification, and not adapt to special cases arising from bugs, obsolescence, and inconsistencies of third-party libraries [...] In addition, node v12.0-12.7 does not support conditional export so the main field will be used.

So I tested this in node 12.0 and as you say, the conditional export isn't used and so the following code fails to work in older versions of node where the main field is used instead of the export map:

const {default: axios} = require('axios');
axios.get('http://www.google.com').then(result => console.log(result.data));

Fails with:

import axios from './lib/axios.js';
       ^^^^^
SyntaxError: Unexpected identifier

But node versions 12.0-7 were never LTS versions, and they are certainly out of date, so maybe we don't need the main field at all if your intention is to not support special cases as a result of obsolescence.

I guess my question is, if we are relying on the exports map for this functionality to work correctly in node, what is the intended use-case for setting the "main" entrypoint to point to the esm version of the package? The field will never be used in current versions of node and breaks in old versions, so maybe it is because that is what would be used by bundlers that don't support the exports map? If that's the case, those same bundlers support the proprietary "module" and "browsers" entry-points.

There's also a ton of other entries already in this package.json for supporting use-cases outside of the npm specificaton:
https://github.com/axios/axios/blob/7a60e50ab97860a908cee831f99d00a1e3b4ef0c/package.json#L140-L146C32

I can maybe see the case for using:

"browser": {
  ".": "./index.js",

instead of "module": "index.js" assuming the two would be equivalent if you didn't wan't to add another non-standard field, but I still think setting the main field to point to commonjs is within specifications and improves compatibility.

@thebanjomatic
Copy link
Author

@DigitalBrainJS I have worked around this issue on my end by removing my dependency on resolve/browser-resolve. I still think it would be a beneficial change to make, but if you disagree, feel free to close this PR. Thanks!

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

2 participants