Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

Correct jsnext:main #144

Merged
merged 1 commit into from
Feb 19, 2017
Merged

Correct jsnext:main #144

merged 1 commit into from
Feb 19, 2017

Conversation

piuccio
Copy link

@piuccio piuccio commented Aug 15, 2016

According to jsforum/jsforum#5 jsnext:main should contain the transpiled version with the exception of ES6 modules.

Users can include the library without having to apply the babel transformations needed by this package

@TrySound
Copy link

I recommend you also to add same module field which is supported and with rollup-plugin-node-resolve and with webpack.

@piuccio
Copy link
Author

piuccio commented Aug 15, 2016

@TrySound thanks for the comment, added


export default Object.assign(base, {
dest: pkg['jsnext:main'],
sourceMap: path.resolve(pkg['jsnext:main']),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better rely on new field :) We should reduce jsnext touching as much as it possible, caz it's legacy.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you may use targets option which is an array of different write options.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for targets option, if that allows for a single Rollup config.

@developit
Copy link
Member

I'm comfortable with the suggested approach, just wondering if it could be accomplished via a single rollup.config.js.

@piuccio
Copy link
Author

piuccio commented Aug 15, 2016

Didn't know about targets. It's now everything in the same rollup file

@@ -47,5 +43,9 @@ export default {
include: 'node_modules/**',
exclude: '**/*.css'
})
],
targets: [
{ dest: pkg.main, sourceMap: path.resolve(pkg.main), format: 'umd', moduleName: pkg.amdName },

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@developit Why not just sourceMap: true?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't know, the original config had that path.resolve. Let me try what happens with sourceMap: true

@piuccio
Copy link
Author

piuccio commented Aug 15, 2016

sourceMap: true has the same effect of manually specifying the path.

Do you guys prefer if I squash all commits together?

@TrySound
Copy link

TrySound commented Aug 15, 2016

@piuccio sourceMap can be 'inline', truely and falsy. So yes, replace with true and squash please.

@TrySound
Copy link

Ah, I see. Cool.

According to jsforum/jsforum#5 jsnext:main should contain the transpiled version with the exception of ES6 modules.

Users can include the library without having to apply the babel transformations needed by this package
@piuccio
Copy link
Author

piuccio commented Aug 15, 2016

done 😃

@developit
Copy link
Member

How certain is the ES5 requirement here? I haven't finished reading through that thread, but my understanding so far is that ES2015 (without stage-xx or any one-off plugins) is assumed to be acceptable if the source is already using ES2015 modules. Neither preact nor preact-compat use react-specific stuff, it's all just things from the es2015 preset (intentionally only using the spec'd stuff). Thoughts?

@piuccio
Copy link
Author

piuccio commented Aug 15, 2016

I'm having problems because preact-compat includes code like this which isn't ES2015 so it's better to transpile it here rather in the clients.

As for the preset es2015, it might be unnecessary, but I guess it benefits lot of people that don't run babel on their node_modules folder but still expect the code to work in most browsers.

@TrySound
Copy link

TrySound commented Aug 15, 2016

@developit Do you support IE?

@piuccio
Copy link
Author

piuccio commented Aug 15, 2016

@TrySound great question, let's see. My project has to work on IE9+

@TrySound
Copy link

In this case module should have es5+modules entry point to prevent long-time vendor transpiling.

@developit
Copy link
Member

@piuccio Ahh - here I thought there was no JSX in preact-compat - IMO that's actually a bug, but I see your point. One trade-off this makes though, is that for those transpiling jsnext:main / modules:main, you end up with multiple copies of each Babel helper.

@piuccio
Copy link
Author

piuccio commented Aug 15, 2016

well if you get rid of JSX in the source code, I might be able to use jsnext:main without this change.

Ultimately I want to tree shake preact and preact-compat. At the moment they're adding 8kb to my project and I'm assuming it can go down.

@developit
Copy link
Member

I will definitely get rid of the JSX. Everything else in there should just be es2015 (via babel, buble, etc - as per the spec). I was debating whether to do that via a transpile step, but really it's only used in one place so I'll probably just use hyperscript directly.

@developit developit closed this in 26c3283 Aug 24, 2016
@developit
Copy link
Member

@piuccio Didn't mean to auto-close this issue, but I did just remove all the JSX and add modules:main. Let me know if that lets you use jsnext:main! The codebase should now work fine with bublé or babel --presets es2015 (no stage-xx)

developit added a commit that referenced this pull request Aug 24, 2016
@developit
Copy link
Member

@piuccio heads up, I've released the changes in 3.0.0. Take a look and let me know if it's working! 😊

@piuccio
Copy link
Author

piuccio commented Aug 24, 2016

Thanks, I'm now on holiday, will let you know next week

@developit
Copy link
Member

Looks like @megafirzen is running into the same sort of issue now that we're publishing a "modules" pointer. I'll manually merge this PR and start publishing .es.js :)

@piuccio
Copy link
Author

piuccio commented Sep 3, 2016

Sorry for the delay, I managed to test it right now and I still have some troubles.

JSX is gone but there are a couple of arrow functions here and there (like here) which break my rollup when I use module: true.

So for the moment I'm stuck with jsnext and module set to false.

@TrySound
Copy link

TrySound commented Sep 3, 2016

@piuccio How's arrow functions breaks rollup? Do you use uglify? Rollup uses acorn which can parse arrow functions and whole es2015.

@piuccio
Copy link
Author

piuccio commented Sep 3, 2016

Yes it breaks the uglify step

@TrySound
Copy link

TrySound commented Sep 3, 2016

@piuccio Then enable babel or buble transpiller by yourself.

@piuccio
Copy link
Author

piuccio commented Sep 3, 2016

@TrySound yeah I might enable babel on node modules but it's a bit weird that I have to compile my dependencies. It certainly makes compiling slower. I might give it another go when I have more time

@TrySound
Copy link

TrySound commented Sep 3, 2016

It's @developit decision. And I'm not against. Now we can use es6 in browsers. For minifying can be used babili. If you want to support older browsers, you should by yourself prepare that code. es6 is standard, not extension.

@piuccio
Copy link
Author

piuccio commented Nov 17, 2016

Closing this, I'm not sure using babel on node_modules is the best solution but at least it's a solution

@piuccio piuccio closed this Nov 17, 2016
@developit developit reopened this Feb 19, 2017
@developit developit merged commit f7e3760 into preactjs:master Feb 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants