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 module field in package.json #928

Closed
Ashot-KR opened this issue Mar 11, 2017 · 7 comments
Closed

Add module field in package.json #928

Ashot-KR opened this issue Mar 11, 2017 · 7 comments

Comments

@Ashot-KR
Copy link
Contributor

Since v1.3.0 inferno and it's packages has dist-es folder, so why don't add a module field to import bundled Inferno es2015 module with import Inferno from 'inferno'? At this moment i should use import Inferno from 'inferno/dist-es'.

By the way: most installed inferno packages has node_modules folder with inferno-shared in it. What's the point of this? why it not installed as dependency to projects node_modules folder? Now i have installed inferno and 4 inferno packages and each has it's own inferno-shared, it looks strange to me.

And thanks for your work, Inferno is great.

@Havunen
Copy link
Member

Havunen commented Mar 11, 2017

Because many users started reporting errors when they used ES6 modules. So I simply dropped the module field. You can use it at your own risk, we don't currently support it because module entry point is undocumented feature.

@Ashot-KR
Copy link
Contributor Author

Ashot-KR commented Mar 11, 2017

I've made experiment:

  1. i added "module": "dist-es/index.js" to installed inferno and it's packages manually
  2. tried to bundle project and get error warning' is not exported by node_modules/inferno/node_modules/inferno-shared/dist/inferno-shared.node.js because each package has it's own inferno-shared module and inferno-shared.node.js do not export warning as it cjs module
  3. i removed node_modules from all packages, installed inferno-shared to project and added "module": "dist-es/index.js" to it package.json
    and my project bundled successfully (except some warning during bundle process). I used rollup bundler.

Anyway i can't claim that Inferno really need in module(or jsnext:main) field. Thanks for your answer!

@Havunen
Copy link
Member

Havunen commented Mar 12, 2017

Btw 'inferno:main' is pointing to es6

@TrySound
Copy link

@Havunen I'm maintainer of rollup-plugin-node-resolve. If there's a problem, let's fix it. Bundling commonjs is not efficient for libraries. As you can see redux uses this too reduce bundle size.
reduxjs/redux#2283

@longlho
Copy link
Member

longlho commented Mar 12, 2017

@TrySound problem is that people don't configure rollup/webpack to transpile ES6 in node_modules. Webpack also has a different mechanism of wrapping default-exported module that uses defineProperty, making getter/setter injection broken. Not to mention TS ecosystem as well:

See:
#903
#686

It's more than just the rollup ecosystem

@teehemkay
Copy link

teehemkay commented Apr 8, 2017

@Ashot-KR @Havunen

FWIW, I've noticed that code in inferno/dist-es/**/*.js imports from inferno-shared.
But shouldn't they instead import from inferno-shared/dist-es?

@LukeSheard
Copy link
Contributor

LukeSheard commented Jun 10, 2017

@teehemkay If you're resolving inferno:main as your entry point in something like web pack then inferno-shared will refer to inferno-shared/dist-es. Since we now support the inferno:main tag for ES6 I'm going to close this. If people still have strong opinion about this feel free to reopen.

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

No branches or pull requests

6 participants