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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add module, jsnext:main to package.json #249

Closed
wants to merge 1 commit into from

Conversation

aviddiviner
Copy link

Hey. I'm using your excellent logger in a package which I build using rollup. I grab the ES6 sources from various node_modules and then bundle everything together in a single JS file which I serve up.

In order for rollup to do its thing properly, it needs an idea of where the ES6 sources live, which it gets from this value in your package.json file. So, if you'd like, here's that 2-line change to make it happen.

Here's a bit of background on the pkg.module property.

If you want to merge it back into the main project that would be nice, then I don't have to maintain my own fork. 馃槃 Thanks!

@codecov-io
Copy link

codecov-io commented Jul 19, 2017

Codecov Report

Merging #249 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #249   +/-   ##
=======================================
  Coverage   83.56%   83.56%           
=======================================
  Files           5        5           
  Lines         146      146           
=======================================
  Hits          122      122           
  Misses         24       24

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 38d48a0...f835895. Read the comment docs.

@imevro
Copy link
Collaborator

imevro commented Jul 19, 2017

Hi! We already tried it, but failed and ended on #234

I didn鈥檛 merge it yet because I鈥檓 not familiar with rollup and we need test, but I don鈥檛 had enough time. Maybe on this week I can finish this long drama.

Is this PR related to yours?

@aviddiviner
Copy link
Author

Ah okay. I'm not super familiar with the inner workings of rollup, but this change works for me. That other PR looks a little heavier. I just pointed the pkg.module at the source, since that's ES6 already and it worked :)

If you have other changes in the pipeline, then feel free to discard this PR, I have my own fork anyway.

@thiamsantos
Copy link
Contributor

The problem of just adding the pkg.module without bundling it with babel is that the majority of the people ignore the node_modules on transpilation. Also babel-loader recommend to use in that way. In that way you will end up with a ES6 code in your bundle, what will break uglify and not work on older browsers. That's why I think #234 is the best solution.

@imevro
Copy link
Collaborator

imevro commented Aug 16, 2017

Moved to #234

@imevro imevro closed this Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants