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

Updated to use ECMAScript 2015 Object.assign. #14

Merged
merged 1 commit into from
Dec 19, 2018

Conversation

mark-atk
Copy link
Contributor

@mark-atk mark-atk commented Dec 4, 2018

Updated pbxproj.js to use ECMAScript 2015 Object.assign to avoid Maximum call stack size exceeded error when running react-native link.

I was getting the above error when running link in the Ignite Bowser template. This fix resolved that. Object.assign targets ECMAScript 2015 though so I see how that might be an issue if you want to target previous versions.

Updated pbxproj.js to use ECMAScript 2015 Object.assign to avoid Maximum call stack size exceeded error when running react-native link.
@janpio
Copy link
Member

janpio commented Dec 4, 2018

Thanks for the PR!

I was getting the above error when running link in the Ignite Bowser template.

Can you elaborate on that? Those names mean nothing to me.

@mark-atk
Copy link
Contributor Author

mark-atk commented Dec 5, 2018

Yea sure, Ignite Bowser is a popular boilerplate template for React Native. It just helps you spin up a project with all the essential dependencies and boilerplate code for a proper (read not prototype) project.

You can see they have an issue open here: infinitered/ignite-bowser#120

I will link this PR in the issue.

@janpio
Copy link
Member

janpio commented Dec 5, 2018

Ok thanks. Can you point me to where this library here is defined and used as a dependency in this boilerplate code? I am a bit confused by the stacktrace at merge_obj (/Users/xxxx/node_modules/xcode/lib/parser/pbxproj.js:1883:25).

@brodycj brodycj mentioned this pull request Dec 5, 2018
11 tasks
@brodycj
Copy link

brodycj commented Dec 5, 2018

Another thing is that the proposed change would require us to drop support for older Node.js versions, which would need to be done in a major release, as I proposed in #20. (There may still be some non-Cordova using this package on older Node.js versions, which we should not break according to semver etiquette.)

I am also looking forward to the answer to the latest question by @janpio.

P.S. I found a "ponyfill" at https://github.com/sindresorhus/object-assign in case it may help us with Node.js pre-4.0.

@mark-atk
Copy link
Contributor Author

mark-atk commented Dec 6, 2018

Apologies for the delay in response. You are spot on, it isn't referenced directly in the boilerplate template. I actually went and looked through the dependencies and it looks like the library is a dependency of react-native and of appcenter-link-scripts in my specific project.

There are quite a few of these maximum call stack size exceeded errors in the react-native repo. Some related to react-native link and one that seems related to this library: Issue 12188. Honestly the information in that issue is all over the place and some of those solutions seem more like work arounds than anything else.

I will probably have more time over the weekend to investigate what exactly was causing this error in my environment. At this point I can't be sure that the fix in this PR resolves the underlying issue, it just might be moving it further downstream.

However it does help remove a good 12 lines of code ;) Whether that is worth the risk of dropping support for older version of Node is up for discussion.

@brodycj
Copy link

brodycj commented Dec 19, 2018

Travis CI passes if I merge this change in my own workarea: https://travis-ci.org/brodybits/cordova-node-xcode/builds/470183332, merging now.

@brodycj brodycj merged commit b1ff4cc into apache:master Dec 19, 2018
@brodycj
Copy link

brodycj commented Dec 19, 2018

@RokoBasilisk I did a squash merge so your master branch has now diverged from this project. I highly recommend the following:

  • In your master branch do git reset HEAD~1 to go back to the previous commit
  • use git pull to pull the latest changes from the master branch of this project
  • do git push with -f or --force flag to push the latest changes to your master branch on GitHub
  • In the future please make a new branch name before raising a PR to avoid this kind of an issue in the future.

@janpio
Copy link
Member

janpio commented Dec 19, 2018

(Or just get rid of your fork and create a new one if you need it again 🙊)

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.

3 participants