-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Use yarn to manage dependencies #5527
Conversation
I'm curious to see how this impacts build times. Less deps are good, but we're already at 7s of install time, so we're pretty close to optimal: https://travis-ci.org/ReactTraining/react-router/builds/274603368 To be clear, I'm totally on board with using yarn workspaces, but let's not make perf a huge motivating factor. Features are more important, as long as the tradeoff isn't some huge regression (which we'll see the impact of after Travis builds up a cache). Since the website is now at the top level, can we stop building it during CI? It doesn't have any tests, so it's kind of a wasted effort. We could exclude it from the workspace, since it's its own separate thing anyways. That'll cut down a YUGE number of deps as well. |
Yes, absolutely. Let's do something in a separate PR. |
Can you do that here? I believe you just need to remove it from the |
Nah, I've been messing with yarn workspaces + jest + react native for the past 24 hours and I feel like I'm getting so close but no cigar yet. I'd rather just remove the website from the build on current master and then rebase this work. |
The website uses the current react router code on master, its a great way to gut-check changes in addition to the automated tests. How will that work if it's not in the "workspaces"? |
I'm not sure I understand. It only runs the webpack build. There is no further testing or usage of the build artifacts. It's a wasted effort as far as I can tell. |
Would you two mind discussing in #5529? I'd like to keep the conversation focused on moving to yarn in this PR. 😅 Thanks! |
Re: #5502 I think you can add jest to the top-level package and try changing the test command to |
For completeness, here's what I get after a fresh
Would love it if someone who knows more about jest wouldn't mind taking a look. |
I should also note that I've tried adding It may be helpful to look through jest's /cc @cpojer Is it obvious to you what I'm doing wrong here? |
From this jestjs/jest#2488 (comment), I added the following to my "jest": {
"preset": "react-native",
"transformIgnorePatterns": [
"node_modules/?!(react-native|native-base)"
]
} I'm not sure why that fix is necessary, since as far as I know the |
Could |
Hm, I don't know what's going on here. I would assume that merging the react-native preset is causing trouble. Since Jest now supports JS files as config, I would probably suggest trying to not use the |
I took advantage of the
I don't know why this is happening. I did a fresh clone of the repo just to verify that locally |
Thanks for doing the digging, @pshrmn. I really, really appreciate it 😅 And thanks for your comments, @cpojer. I think you nailed it. I'm going to experiment with a custom jest config for |
Well, I tried using a custom jest config with no luck. But then I backed up and tried just running the react-router-native app and the react-native packager had problems finding the dependencies it needed as well, so I realized this isn't just an issue with jest + yarn workspaces, it's a RN issue as well! Issues like this are probably why yarn workspaces are still considered experimental. Hoisting all the deps into a common |
Let's use yarn workspaces to manage dependencies (see #4362). This should improve install times by reducing the number of packages we need to install.
An alternative approach could be to use lerna's
--hoist
feature if we want to stick with thenpm
client. But I'm using yarn pretty much everywhere these days, so I'd like to just go with that.Currently, I can't get the
react-router-native
tests to pass using yarn. It seems jest is looking for stuff inpackages/react-router-native/node_modules
that may not be there because yarn workspaces puts them in the rootnode_modules
dir. At least that's my guess./cc @rogeliog who submitted #5502 that uses jest's "projects" feature for testing. I wonder if that would help here...