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

Make tests pass using Node 5 / NPM 3 #832 #840

Closed
wants to merge 22 commits into from

Conversation

raineroviir
Copy link
Contributor

No description provided.

@raineroviir
Copy link
Contributor Author

ugh I'm having some trouble merging all these commits .. I might give up and submit a new pr

@kassens
Copy link
Member

kassens commented Feb 17, 2016

No worries, when we merge this, it will all be combined into one commit.

@raineroviir
Copy link
Contributor Author

Ok cool thanks!
On Wed, Feb 17, 2016 at 2:41 PM Jan Kassens notifications@github.com
wrote:

No worries, when we merge this, it will all be combined into one commit.


Reply to this email directly or view it on GitHub
#840 (comment).

"<rootDir>/node_modules/react/",
"<rootDir>/node_modules/react-dom/",
"<rootDir>/node_modules/react-static-container/"
"<rootDir>/node_modules/(?!(fbjs/lib/ErrorUtils.js$|fbjs/lib/fetch.js$|fbjs/lib/fetchWithRetries.js$))"
Copy link
Member

Choose a reason for hiding this comment

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

If you keep the deleted lines here and just have 2 patterns and add 4 and 5 to the travis config, do the tests pass in both environments? It'd be nice to work in both if it's just adding the new patterns instead of replacing them.

Is there something that makes it difficult to keep the npm 2 patterns in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only difficulty is I don't know how to. Let me see what I can do

@kassens
Copy link
Member

kassens commented Feb 19, 2016

This is great, thanks for taking the extra step for cross compatibility! Just one more thing

@@ -30,7 +30,8 @@
"dependencies": {
"babel-runtime": "5.8.24",
"fbjs": "^0.7.0",
"react-static-container": "^1.0.0-alpha.1"
"react-static-container": "^1.0.0-alpha.1",
"babel-relay-plugin": "0.7.1"
Copy link
Member

Choose a reason for hiding this comment

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

I think the idea is that people would need to add babel-relay-plugin to their project with npm 3, so it should remain (just) a peer dependency. I'd love someone with more npm knowledge here..

cc @steveluscher

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Edit: If the plugin isn't under dependencies, you have to manually install it with npm 3.
Which means npm test will fail in travis-ci since the plugin wasn't installed. You can see the failing tests above for one of my commits: https://travis-ci.org/facebook/relay/builds/109438390

Copy link
Member

Choose a reason for hiding this comment

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

Just guessing here, but should it maybe be in devDependencies in that case? It's needed to run the unit tests, but if you're using it in your own project, you should add it to your dependencies (to use it in your transform step).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, let me try it out
On Fri, Feb 19, 2016 at 6:28 PM Jan Kassens notifications@github.com
wrote:

In package.json
#840 (comment):

@@ -30,7 +30,8 @@
"dependencies": {
"babel-runtime": "5.8.24",
"fbjs": "^0.7.0",

  • "react-static-container": "^1.0.0-alpha.1"
  • "react-static-container": "^1.0.0-alpha.1",
  • "babel-relay-plugin": "0.7.1"

Just guessing here, but should it maybe be in devDependencies in that
case? It's needed to run the unit tests, but if you're using it in your own
project, you should add it to your dependencies (to use it in your
transform step).


Reply to this email directly or view it on GitHub
https://github.com/facebook/relay/pull/840/files#r53542588.

@cpojer
Copy link
Contributor

cpojer commented Feb 20, 2016

fyi jest 0.9.0 (without the fb-suffix) will have fixed npm3 support in Jest. I'll try to get it out next week :)

@raineroviir
Copy link
Contributor Author

OK great! I moved the babel-relay-plugin into devDeps and you were right @kassens!
Also, Travis-CI is failing a test that will be fixed in #810

@@ -92,7 +93,8 @@
"<rootDir>/node_modules/fbjs/lib/(?!(ErrorUtils.js$|fetch.js$|fetchWithRetries.js$))",
"<rootDir>/node_modules/react/",
"<rootDir>/node_modules/react-dom/",
"<rootDir>/node_modules/react-static-container/"
"<rootDir>/node_modules/react-static-container/",
"<rootDir>/node_modules/(?!(fbjs/lib/ErrorUtils.js$|fbjs/lib/fetch.js$|fbjs/lib/fetchWithRetries.js$))"
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to revert this change once I update to jest 0.9.0-fb3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok sure! I'll be awaiting jests 0.9.0-fb3 release

ghost pushed a commit that referenced this pull request Feb 24, 2016
Summary:0.9.0-fb3 comes with all those sweet npm3 fixes. Also see #840 and #832.

Tests are passing for me on node4 both with npm2 and npm3.

See jestjs/jest@7b44ca0 and jestjs/jest@2d10421

cc kassens raineroviir
Closes #877

Differential Revision: D2971806

fb-gh-sync-id: 85f2898f71df12eaf7f2b4bd6a2e641305453b45
shipit-source-id: 85f2898f71df12eaf7f2b4bd6a2e641305453b45
@kassens
Copy link
Member

kassens commented Feb 25, 2016

Looks like the flow fix didn't quite work. Sorry this is such a journey and thank you for keeping the updates coming!

@raineroviir
Copy link
Contributor Author

Yeah! Its a big project, not easy! We can either ignore the 'base62' node_module or include it under Flow, in the flowconfig which one makes more sense? (Both would fix the travis error)

@raineroviir
Copy link
Contributor Author

Getting this error on npm install:

Error: ModuleNotFoundError: Module not found: Error: Cannot resolve 'file' or 'directory' ./Set in /Users/raineroviir/relay/lib.

as well as this on npm test:

src/query/buildRQL.js:25
 25: const Set = require('Set');
                 ^^^^^^^^^^^^^^ Set. Required module not found
Found 1 error

Seems like ES6 Set isn't getting loaded?

@josephsavona
Copy link
Contributor

@raineroviir that's from a commit of mine yesterday, i'm trying to figure out why Set isn't available and will report back here.

@josephsavona
Copy link
Contributor

see #881 for the fix to the missing Set module

@raineroviir
Copy link
Contributor Author

Thanks Mr. Savona

@kassens
Copy link
Member

kassens commented Feb 26, 2016

Awesome, great to see it finally all working 👍

@facebook-github-bot import

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@cpojer
Copy link
Contributor

cpojer commented Feb 26, 2016

Yes! This is great.

@raineroviir
Copy link
Contributor Author

Glad to help =)

This pull request was closed.
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.

None yet

5 participants