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

Enable Object Rest Spread by default #1835

Merged
merged 5 commits into from Aug 11, 2018

Conversation

@DeMoorJasper
Copy link
Member

This is not a standard yet, not sure if this is a good idea

@arv
Copy link
Contributor Author

arv commented Aug 2, 2018

@DeMoorJasper It is part of ES2018 as I already mentioned in the PR description. More points that it is worth enabling this now:

https://github.com/tc39/proposals/blob/master/finished-proposals.md

Shipping in Chrome, Firefox, Safari (both macOS and iOS), V8: http://kangax.github.io/compat-table/es2016plus/#test-object_rest/spread_properties

@arv
Copy link
Contributor Author

arv commented Aug 2, 2018

Now I need to find out why it passes locally but not on the Travis/AppVeyor

@DeMoorJasper
Copy link
Member

@arv you still have to add it as a dependency if I'm not mistaken. That's probably the reason

Copy link
Member

@DeMoorJasper DeMoorJasper left a comment

Choose a reason for hiding this comment

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

Please fix the tests

@arv
Copy link
Contributor Author

arv commented Aug 9, 2018

@DeMoorJasper I need to figure out the test failures. Any ideas?

@DeMoorJasper
Copy link
Member

@arv Unfortunately I don't have any clue why it's failing

@arv
Copy link
Contributor Author

arv commented Aug 9, 2018

I can look at it tomorrow.

@@ -28,6 +28,21 @@ describe('javascript', function() {
assert.equal(output.default(), 3);
});

it('should produce a basic JS bundle with object rest spread support', async function() {
let b = await bundle(__dirname + '/integration/object-rest-spread/object-rest-spread.js');
Copy link
Member

Choose a reason for hiding this comment

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

It wasn't bundling anything, got it it bundle correctly in the test now, not sure how this worked locally. Now it still throws an error though.

Hope you can resolve it :)

@@ -265,6 +265,14 @@ async function getEnvPlugins(targets, useBuiltIns = false) {
{},
{targets, modules: false, useBuiltIns: useBuiltIns ? 'entry' : false}
).plugins;

// babel-preset-env version 6.x does not cover object-rest-spread so always
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not seem like the right place for this but I'm not sure where would be a better place to put this?

Copy link
Member

Choose a reason for hiding this comment

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

It should be fine here, but than you should probably remove it from JSAsset.js as now it probably runs it twice...

Copy link
Member

Choose a reason for hiding this comment

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

On sidenote though, this would also run over node_modules, not sure if that's intended

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be fine here, but than you should probably remove it from JSAsset.js as now it probably runs it twice...

The one in JSAsset.js seems to only be used for parser options. Inspecting the plugins array we get from getEnvPlugins and it does not contain babel-plugin-transform-object-rest-spread

On sidenote though, this would also run over node_modules, not sure if that's intended

That is what I want since npm modules are now starting to use this feature.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but babylon runs before the other transforms to create the ast, so this would run twice, at different places, but it'll still run twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. Thanks.

@DeMoorJasper DeMoorJasper merged commit 81151bb into parcel-bundler:master Aug 11, 2018
@arv
Copy link
Contributor Author

arv commented Aug 11, 2018

Thanks

@arv arv deleted the object-rest-spread branch August 11, 2018 04:45
@danielo515
Copy link

Is this released ?

@ashinzekene
Copy link

No

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

4 participants