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

Support ES.Next object rest spread in config files like rollup.config.js #1759

Merged
merged 1 commit into from
Dec 5, 2017

Conversation

haraldrudell
Copy link

@haraldrudell haraldrudell commented Nov 24, 2017

fixes #1758

test preparation:

# checkout rollup
npm run build
cat <<EOF >test.js
const a = {b: 1}
const c = {...a}
EOF

test execution:
./bin/rollup --config ./test.js

if it works:
           [!] Config file must export an options object, or an array of options objects
           https://github.com/rollup/rollup/wiki/Command-Line-Interface#using-a-config-file

if it fails:
           [!] Error: Unexpected token
           test.js (2:11)
           1: const a = {b: 1}
           2: const c = {...a}
                         ^

@nathancahill
Copy link
Contributor

This doesn't fix #1757. This PR only applies to es.next syntax in the config.

@lukastaegert
Copy link
Member

Thanks @nathancahill, I have taken the liberty to change that in the description 😉.
@haraldrudell I am against doing any automatic transpilation of the actual sources but for the config file maybe this is actually ok. Any other opinions?

If we include this, we should probably keep the bublé transformations even once object rest spread has reached stage 4 and is officially supported by acorn to not introduce a breaking change for users who opted to use even more unsupported features (whatever those are). Just something to consider.

@lukastaegert
Copy link
Member

Since there were no other opinions voiced, I'll put this into the next patch release.

@feross
Copy link

feross commented Jan 15, 2018

This PR seems to break any config file that uses tagged template strings, which buble cannot handle. This was working fine before this PR.

Related to: #1875 and #1873

@haraldrudell
Copy link
Author

It is true this might be bad, since bublé transpilation lacks many ECMAScript ES.Next features, in particular async breaks havoc.

Instead once can transpile rollup.config.js ahead of time:

rollup.config.js can be transpiled using es2049scripts from the ECMAScript 2049 package
https://github.com/haraldrudell/ECMAScript2049/tree/master/workspace/packages/es2049scripts
https://www.npmjs.com/package/es2049scripts

An example of this is here rollup.config.mjs:
https://github.com/haraldrudell/ECMAScript2049/blob/master/workspace/packages/es2049package/configes/rollup.config.mjs

Another interesting project is zero-configuration RollupJS provided by es2049package:
https://github.com/haraldrudell/ECMAScript2049/tree/master/workspace/packages/es2049package
https://www.npmjs.com/package/es2049package

Here is an example of transpiling with RollupJS configuration-free using es2049package:
https://github.com/haraldrudell/ECMAScript2049/blob/master/workspace/packages/allspawn/package.json

Documentation on how to use RollupJS without rollup.config.js
https://github.com/haraldrudell/ECMAScript2049/tree/master/workspace/packages/es2049package#usage

I wanted to tell @Rich-Harris and @lukastaegert about this.

No one ever again have to write a single line that is not ECMAScript ES.Next
Quote Rudell

@lukastaegert
Copy link
Member

I guess we should remove bublé again at some point. Once #1857 is released (which I plan for the next non-patch release), we could just use this to add object rest spread manually.

@haraldrudell
Copy link
Author

haraldrudell commented Jan 17, 2018

And object spread properties is in Node.js natively since v8.6, so the reason this was put in is no longer valid.

@adrianheine
Copy link
Contributor

@haraldrudell Not everyone is already using Node >= 8.6.

@feross
Copy link

feross commented Jan 18, 2018

Sidenote: why does rollup take on the responsibility of transpiling the config file? This seems like overkill. If I'm running a version of node that doesn't support a certain ES feature, I don't really have an expectation of those features working.

If I do want new ES features to work, I can use @babel/register or manually transpile the file.

@lukastaegert
Copy link
Member

Actually I agree. Object rest spread was the single one thing people kept wishing for config files + there was this PR and I did not expect bublé to cause so much trouble. Now Object rest spread can be easily approximated with Object.assign, so maybe it is for the best to remove this change in one of the next "major" releases again as a (small) breaking change?

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

Successfully merging this pull request may close these issues.

Support object rest spread operator in rollup.config.js
5 participants