andreypopp / autobind-decorator

Decorator to automatically bind methods to class instances
MIT License
1.45k stars 66 forks source link

package.json module field points to uncompiled source #78

Closed lroling8350 closed 5 years ago

lroling8350 commented 5 years ago

v2.3.1 introduced the env-preset for babel breaking our builds because we do not include it. I found the issue https://github.com/andreypopp/autobind-decorator/issues/75 but was unable to comment so I'm creating a new one. I have read over https://babeljs.io/blog/2018/06/26/on-consuming-and-publishing-es2015+-packages which you are giving as the reason for why it points to uncompiled source. However, in the article it states

"We should continue to publish ES5/CJS under main for backwards compatibility with current tooling but also publish a version compiled down to latest syntax (no experimental proposals) under a new key we can standardize on like main-es. (I don't believe module should be that key since it was intended only for JS Modules)"

Which indicates that module is not the correct place to do this due to backwards compatibility. We use modules before main in our webpack to get the benefits of import statement treeshaking but still require the rest to be es15 compatible.

Is it possible to have the module point to an es5 compatible version with imports instead of requires as is intended for the module field?

stevemao commented 5 years ago

We kept main in package.json. The main entry still points to the original build.

Sent from my iPhone

On 20 Nov 2018, at 3:11 am, lroling8350 notifications@github.com wrote:

v2.3.1 introduced the env-preset for babel breaking our builds because we do not include it. I found the issue #75 but was unable to comment so I'm creating a new one. I have read over https://babeljs.io/blog/2018/06/26/on-consuming-and-publishing-es2015+-packages which you are giving as the reason for why it points to uncompiled source. However, in the article it states

"We should continue to publish ES5/CJS under main for backwards compatibility with current tooling but also publish a version compiled down to latest syntax (no experimental proposals) under a new key we can standardize on like main-es. (I don't believe module should be that key since it was intended only for JS Modules)"

Which indicates that module is not the correct place to do this due to backwards compatibility. We use modules before main in our webpack to get the benefits of import statement treeshaking but still require the rest to be es15 compatible.

Is it possible to have the module point to an es5 compatible version with imports instead of requires as is intended for the module field?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

stevemao commented 5 years ago

Which field should be used for standard modern JavaScript?

Sent from my iPhone

On 20 Nov 2018, at 3:11 am, lroling8350 notifications@github.com wrote:

v2.3.1 introduced the env-preset for babel breaking our builds because we do not include it. I found the issue #75 but was unable to comment so I'm creating a new one. I have read over https://babeljs.io/blog/2018/06/26/on-consuming-and-publishing-es2015+-packages which you are giving as the reason for why it points to uncompiled source. However, in the article it states

"We should continue to publish ES5/CJS under main for backwards compatibility with current tooling but also publish a version compiled down to latest syntax (no experimental proposals) under a new key we can standardize on like main-es. (I don't believe module should be that key since it was intended only for JS Modules)"

Which indicates that module is not the correct place to do this due to backwards compatibility. We use modules before main in our webpack to get the benefits of import statement treeshaking but still require the rest to be es15 compatible.

Is it possible to have the module point to an es5 compatible version with imports instead of requires as is intended for the module field?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

Mehdi40 commented 5 years ago

Hey,

I'm +1ing this issue. We encountered almost the same problem. The solution of using "main" is working, but it creates others issues with others dependencies. So, I don't really know if this is possible, but I'm asking the same question as the author of this issue : "module" pointing to the actual "main" would solve my problem.

lroling8350 commented 5 years ago

I'm unsure what should be used for modern java script as i don't think there has been a convention created yet. The article provided by babel alludes to this. It does state that module was chosen by both webpack and rollup as a convention to provide javascript with es6 imports but still compiled to es5 syntax and to maintain backwards compatibility should likely not be used. We got this dependency intrinsically from react-dnd-html-backend but discovered moving a major version removed the dependency. We were able to upgrade react-dnd and this is no longer an issue for me personally but it seems others are running into it as well.

To give some context. I help manage some core libraries, one of which was including this dependency via react-dnd so all our projects started breaking. Many teams who leverage our libraries are not very strong in babel and webpack (use some precanned templates) so this became a quick blocker. Since it was an intrinsic dependency it was going to be hard to explain to the all the teams why it was broken. We also use the module property ahead of main in our webpack builds to get the benefits of better treeshaking. So when you say we should just use main it means we would need to give up using module all together or for us to disseminate to all the teams how and why to add excludes for this library.

For a while we were providing non-compiled source as a solution, We solved the problem by having people just access source directly like so

import { myFunc } from 'my-library/src'

It is a bit ugly but did the trick and it was clear we were grabbing source directly. Ideally a better convention will come about at some point.

stevemao commented 5 years ago

I still think that compiling node_modules should be the ultimate solution. babel 7 has really good support for that and webpack has already dropped uglifyJS. And IE11 usage is dropping...

vlad-ivanov-d commented 5 years ago

Faced with the same problem, because one of my solution's dependency use this package. I don't use babel, I'm using typescript and webpack. In this case I have to install babel only for workaround this issue - it's crazy.

Possible solution: such significant changes (as broken es5 support) should be done under major version of package. This prevents issues after running "npm install" without package-lock.json file.

stevemao commented 5 years ago

@vladivanovrf https://github.com/andreypopp/autobind-decorator#es5-and-uglify-users

vlad-ivanov-d commented 5 years ago

@stevemao yeah, thank you, I've already fixed the issue by setting mainFields as in your link. And it works in my case. But I'm not sure that such fix won't affect on some other dependencies/project chunks and so on in any cases.

But the main idea of my post was in the second part, just a small advice: not about how to fix the issue, but how to prevent - apply significant changes only in major versions. It will save time for everybody.

lroling8350 commented 5 years ago

I don't think anyone is arguing with you about compiling node_modules. I just don't think the module field was the right field to do it. The babel post you refer to suggests NOT using the module field so it doesn't break people.

stevemao commented 5 years ago

I have changed the module field to ES5 + es modules and added es field for modern JS. We really need some convention for modern JS 😞