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

A trailing comma is not permitted after the rest element - ListView.js 457:14 #10199

Closed
colinlmcdonald opened this issue Oct 1, 2016 · 29 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@colinlmcdonald
Copy link

Issue Description

I wanted to start my first react-native application. I followed the steps in the tutorial to get it installed and running. Without any modifications, after running react-native run-ios and going to Xcode I got the following error:

image

Steps to Reproduce / Code Snippets

Follow the react-native tutorial https://facebook.github.io/react-native/docs/getting-started.html. After deleting the trailing commas in;

ListView.js - line 457,
NavigationCard.js - line 85,
MessageQueue.js - line 390

it worked. Interestingly, these trailing commas were all after using the spread operator (...).

Expected Results

Trailing commas in deconstructed objects should work.

Additional Information

  • React Native version: 0.34.1
  • Platform(s) (iOS, Android, or both?): iOS
  • Operating System (macOS, Linux, or Windows?): macOS
@bijeebuss
Copy link

just hit the same issue

@mlazowik
Copy link
Contributor

mlazowik commented Oct 1, 2016

Just run into the same thing. Worked with

  • babel-polyfill 6.13.0
  • babel-preset-es2016 6.14.0

Broken with

  • babel-polyfill 6.16.0
  • babel-preset-es2016 6.16.0

Investigating further.

@CarringtonCreative
Copy link

Same here

@joncursi
Copy link
Contributor

joncursi commented Oct 1, 2016

Yep. Lots of stuff different packages under node_modules just broke for me with the same error message, not just react-native.

@mlazowik
Copy link
Contributor

mlazowik commented Oct 1, 2016

https://github.com/babel/babylon/blob/1285131e3eb28ab6f5f62a7c5fd3ffd7ce1e5eb8/CHANGELOG.md#v6113-2016-10-01

@npomfret
Copy link
Contributor

npomfret commented Oct 1, 2016

Same here on node v6.3.1 but working on another machine running v6.4

@colinlmcdonald
Copy link
Author

@mlazowik good find, looks like babel has changed the spec for object destructuring. I'm going to make a pull request unless you want to?

@mlazowik
Copy link
Contributor

mlazowik commented Oct 1, 2016

@colinlmcdonald making one right now :)

@CarringtonCreative
Copy link

CarringtonCreative commented Oct 1, 2016

confirming that @colinlmcdonald temp fix works


OSX El Capitan 10.11.6
npm 2.14.4
react 15.3.2
react-native 0.34.1

@ipsy-eric
Copy link

ipsy-eric commented Oct 1, 2016

Since babylon seems to think point releases are ok for fundamental parsing strictness changes, maybe React Native should pin to exact versions of bablyon?

@npomfret
Copy link
Contributor

npomfret commented Oct 1, 2016

What's the fix?

@NicolasLEBRUN-
Copy link

Same problem here.

@CarringtonCreative
Copy link

@npomfret

Deleting the trailing commas in

ListView.js @ line 457,
NavigationCard.js @ line 85,
MessageQueue.js @ line 390

Restart your package manager. If the changes don't take effect then try closing out Xcode and the simulator. Then reopen each of them.

@ipsy-eric
Copy link

ipsy-eric commented Oct 1, 2016

The fix for me is a lot of monkey patching inside node_modules and pausing using continuous build tools for the weekend. And looking forward to the PR @mlazowik is making :)

@CarringtonCreative
Copy link

CarringtonCreative commented Oct 1, 2016

@npomfret

Here are the file paths:

node_modules/react-native/Libraries/CustomComponents/ListView/ListView.js

node_modules/react-native/Libraries/CustomComponents/NavigationalExperimental/NavigationCard.js

node_modules/react-native/Libraries/Utilities/MessageQueue.js

@ipsy-eric
Copy link

@npomfret / @Carrington-Dennis -- lots of other node modules are going to be hit by this too. I suggest reloading the app and following the redbox errors to see where is affected, making PRs to those projects. As an example, react-native-vector-icons also got smacked by this babel change.

@mlazowik
Copy link
Contributor

mlazowik commented Oct 1, 2016

And use shrinkwrap.

@mankins
Copy link

mankins commented Oct 1, 2016

@npomfret try the method @Carrington-Dennis suggests. Just confirmed it works until the rest gets sorted out.

@jkhustler
Copy link

confirming monkey patching works

@hzoo
Copy link
Contributor

hzoo commented Oct 1, 2016

Yeah to confirm this is from babel/babylon#149, babel/babylon#149 (comment) in v6.11.3

Can do a patch now to rollback since it can be too much trouble to change.

@jnmandal
Copy link

jnmandal commented Oct 1, 2016

@Carrington-Dennis last one is

node_modules/react-native/Libraries/Utilities/MessageQueue.js

not:

node_modules/react-native/Libraries/Utilities/ListView/MessageQueue.js

👀

@CarringtonCreative
Copy link

@jnmandal correct. Thank you. I just changed my original reply to

node_modules/react-native/Libraries/Utilities/MessageQueue.js

@hzoo
Copy link
Contributor

hzoo commented Oct 2, 2016

Sorry about that yall - didn't intend to make a bad experience using react-native with those issues.

Released babel/babylon#154 with v6.11.4 which reverts the issue. You'll need to reinstall babylon, which can be done with rm -rf node_modules/babylon and npm install again.

I think for most projects you don't compile node_modules (there's a dist/lib folder used in npm) so you're able to just fix the errors in your own application when it happens and you update. However, if another project's files cause errors it's basically out of your hands unless you change node_modules yourself. (I didn't think of this at the time).

@ipsy-eric
Copy link

ipsy-eric commented Oct 2, 2016

Thanks @hzoo for the quick fix to the issue

@jbernard1
Copy link

@Carrington-Dennis Correct list of paths. Finally got my app working.

@loganfsmyth
Copy link

Keep in mind that while we're reverted this change, code that was broken by this is wrong per the spec and will eventually be a syntax error whenever Babel has a major version bump or introduces an alternate way to version parsing without breaking people this badly.

Like you can't do an array spread like this:

var [ a, b, ...c, ] = [1, 2, 3];

you can't do an object spread like this:

var { a, b, ...c, } = {a: 1, b: 2, other: 3 };

as the current object rest spec defines it: https://sebmarkbage.github.io/ecmascript-rest-spread/#Rest

FrankSalad added a commit to FrankSalad/react-native-progress that referenced this issue Oct 2, 2016
Prevent `Syntax Error react-native-progress/Bar.js: A trailing comma is not permitted after the rest element` see facebook/react-native#10199
@ide
Copy link
Contributor

ide commented Oct 2, 2016

Closing since this issue has been fixed upstream with Babylon for now. Thanks for the context @loganfsmyth -- it makes sense that a trailing comma isn't needed after a rest variable since it will always be the last entry.

This is a codemod to strip the comma:

codemod -m --extensions=js --exclude-paths=node_modules '(\.\.\.[\w$]+)\s*,(\s*\})' '\1\2'

@ide ide closed this as completed Oct 2, 2016
@mlazowik
Copy link
Contributor

mlazowik commented Oct 2, 2016

This regex does not catch all instances, as there can be comment(s) between trailing comma and }. Also this applies only to bindings and assignments, so a = should be somewhere in there.

@npomfret
Copy link
Contributor

npomfret commented Oct 2, 2016

After this experience I think it's a good idea to never use wild cards in any dependency management. It only leeds to difficult to diagnose problems that hit you at unexpected times. The hours I've wasted thinking "I didn't check any code in, why has this stopped working!?". At least with today's problem it just caused our builds just fail, but imagine the situation where a dependency update that got consumed silently at build time introduced a bug into your app. You've done nothing wrong, you didn't make any code changes, yet the app doesn't work. This has happened to me and it took days to figure out.

IMO - Updating dependencies, (no matter how trivial the change) should be done and committed to version control. It make it much easier to reason about failures of this nature. The convenience of wildcards in dependency management can be reproduced with a script that just brings the dependencies up to date.

facebook-github-bot pushed a commit that referenced this issue Oct 5, 2016
Summary:
These are caused by new [syntax checking](https://github.com/babel/babylon/blob/1285131e3eb28ab6f5f62a7c5fd3ffd7ce1e5eb8/CHANGELOG.md#v6113-2016-10-01) introduced by babylon.

"The single rest at the end only applies to binding `let { x, ...y } = obj;` and assignment `({ x, ...y } = obj).`"

I'd say this really should be cherry picked into the stable branch.

**Test plan**
1. install babylon@6.11.3
2. see that things break
3. apply patch
4. things work
5. make sure all instances were fixed (I used `\.\.\..*,.*\n.*=` in IntelliJ regex format—find all ... followed by newline followed by =)

Issue #10199
Closes #10200

Differential Revision: D3974066

Pulled By: javache

fbshipit-source-id: 3f3c1e9df01a3b3bdd61dd3863416c638d3ed98d
oblador pushed a commit to oblador/react-native-progress that referenced this issue Dec 4, 2016
Prevent `Syntax Error react-native-progress/Bar.js: A trailing comma is not permitted after the rest element` see facebook/react-native#10199
@facebook facebook locked as resolved and limited conversation to collaborators May 24, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests