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

[npm] Upgrade to stacktrace-parser 0.1.2, which supports io.js #1738

Closed
wants to merge 1 commit into from

Conversation

ide
Copy link
Contributor

@ide ide commented Jun 24, 2015

stacktrace-parser used to list only Node 0.10 under its list of supported engines. This new version includes Node 1.x and 2.x (i.e. io.js) as well, which addresses the warning during npm install.

There's no problem with using the older version of stacktrace-parser; this just clears the warning.

Test Plan: Run npm install under io.js -- no more warnings about a mismatched Node version.

@frantic
Copy link
Contributor

frantic commented Jun 26, 2015

cc @DmitrySoshnikov

@amasad
Copy link
Contributor

amasad commented Jun 30, 2015

All this and the benefits mentioned on the issue are great. But should we consider the thrash we may cause our users when we make a non-backward compatible jump to iojs? Isn't this one additional thing to install? (assuming most people have node installed)
And then when a new version of Node is available (and iojs is discontinued) we'll need to make a similar update?

cc @sahrens @vjeux @frantic

@ide
Copy link
Contributor Author

ide commented Jun 30, 2015

But should we consider the thrash we may cause our users when we make a non-backward compatible jump to iojs?

Yeah. This is a one-time cost but could be frustrating to some people. I think if it's communicated in advance (e.g. by printing a helpful notice in the console for the duration of one release cycle) that will remove most of the surprise. FWIW I've been using RN with io.js for months and it's been working fine -- largely because most of the JS runs on the device anyway -- so I don't expect many hiccups.

Isn't this one additional thing to install?

I'm not sure. Many developers - especially the type who are using the most modern technologies - may actually have io.js installed. Many people also use nvm which cleanly installs multiple Node/io.js versions and lets you switch between them. So I could see the io.js requirement being convenient or maybe not. I don't know.

And then when a new version of Node is available (and iojs is discontinued) we'll need to make a similar update?

The good news is I believe the answer is no. When io.js merges back with Node, they are going to use the io.js codebase with a few patches from Node and call it Node. So, the migration from Node 0.12 to the merged Node 3.0 (or 4.0 or 5.0 depending on how far io.js progresses) is going to be a much bigger leap than steadily keeping up with io.js and dealing with only one major version jump back to the new Node.


Performance-wise, calling ReactPackager.buildPackageFromUrl on http://localhost:8081/Examples/UIExplorer/UIExplorerApp.ios.includeRequire.runModule.bundle?dev=true goes from ~6400ms on Node 0.10.38 to ~6300ms on Node 0.12.4 to ~5800ms on io.js 2.3.1.

@amasad
Copy link
Contributor

amasad commented Jun 30, 2015

I'm convinced. I know @DmitrySoshnikov is in the process of installing iojs in our internal repos. But I'd also like to get @vjeux and @frantic's take on this.

@vjeux
Copy link
Contributor

vjeux commented Jun 30, 2015

Does the current codebase run with io? If yes, then we should today update the docs to install io instead of node.

I'm pro for this change, but need to make sure that the upgrade path is well documented and smooth

@vjeux
Copy link
Contributor

vjeux commented Jun 30, 2015

Since we only require iojs for jest which is not used by people by default, is there any way to remove jest as a required dependency?

@ide
Copy link
Contributor Author

ide commented Jun 30, 2015

@vjeux:

Does the current codebase run with io? If yes, then we should today update the docs to install io instead of node.

io.js has been working for me. There are some npm install warnings but everything seems to run fine. In this diff I updated a couple of dependencies that explicitly added io.js support in a newer version, like the "ws" package. I split out the docs into a separate diff: #1802.

Since we only require iojs for jest which is not used by people by default, is there any way to remove jest as a required dependency?

jest is a devDependency so it gets installed only for people checking out react-native and running npm install inside of the repository. The CocoaPods setup also runs npm install: https://github.com/facebook/react-native/blob/master/React.podspec#L25 -- we can change this to npm install --production. People running npm install react-native already are getting production dependencies only.

I'm pro for this change, but need to make sure that the upgrade path is well documented and smooth

Here's one proposal: for 0.7.1 the packager checks if you are running io.js < 2.x and if so prints a big banner telling you how to upgrade. Then the next release ships with this diff and perhaps keeps the banner so it's clear if you might have compatibility issues. And then packager and CLI scripts can rely on new V8/io.js features too.

@vjeux
Copy link
Contributor

vjeux commented Jun 30, 2015

Sgtm

@amasad
Copy link
Contributor

amasad commented Jun 30, 2015

Here's one proposal: for 0.7.1 the packager checks if you are running io.js < 2.x and if so prints a big banner telling you how to upgrade. Then the next release ships with this diff and perhaps keeps the banner so it's clear if you might have compatibility issues. And then packager and CLI scripts can rely on new V8/io.js features too.

👍 Patches welcome! 😛

@ide ide changed the title [io.js] Docs, test fixes, npm dep upgrades for io.js [npm][io.js] Upgrade ws to 0.7.2 and use stacktrace-parser that supports io.js Jul 2, 2015
@ide
Copy link
Contributor Author

ide commented Jul 2, 2015

This PR is much narrower in scope now and updates the npm packages. Docs are tracked in #1802 and the test infra changes have already been merged. Banner in the packager is at #1824.

This was referenced Jul 2, 2015
stacktrace-parser used to list only Node 0.10 under its list of supported engines. This new version includes Node 1.x and 2.x (i.e. io.js) as well, which addresses the warning during `npm install`.

There's no problem with using the older version of stacktrace-parser; this just clears the warning.

Test Plan: Run `npm install` under io.js -- no more warnings about a mismatched Node version.
@ide ide changed the title [npm][io.js] Upgrade ws to 0.7.2 and use stacktrace-parser that supports io.js [npm] Upgrade to stacktrace-parser 0.1.2, which supports io.js Jul 29, 2015
@ide ide assigned frantic and unassigned amasad Jul 29, 2015
@ide
Copy link
Contributor Author

ide commented Jul 29, 2015

@frantic, could you take a look at this? The PR just updates stacktrace-parser now, which has been updated with our patches (yours and mine) and published to npm.

@frantic
Copy link
Contributor

frantic commented Jul 30, 2015

👍

@frantic
Copy link
Contributor

frantic commented Jul 30, 2015

@facebook-github-bot import

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1612231222350059/int_phab to review.

@frantic
Copy link
Contributor

frantic commented Aug 17, 2015

Should be in with next sync

ide added a commit that referenced this pull request Aug 26, 2015
Summary:
stacktrace-parser used to list only Node 0.10 under its list of supported engines. This new version includes Node 1.x and 2.x (i.e. io.js) as well, which addresses the warning during `npm install`.

There's no problem with using the older version of stacktrace-parser; this just clears the warning.

Closes #1738
Github Author: James Ide <ide@jameside.com>
@ide ide deleted the iojs branch August 27, 2015 09:07
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.

5 participants