Upgrade to Rush 5 #6242
Upgrade to Rush 5 #6242
Conversation
| // Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. | ||
| // See the @microsoft/rush package's LICENSE file for license information. | ||
| Object.defineProperty(exports, "__esModule", { value: true }); | ||
| // THIS FILE WAS GENERATED BY A TOOL. ANY MANUAL MODIFICATIONS WILL GET OVERWRITTEN WHENEVER RUSH IS UPGRADED. |
There was a problem hiding this comment.
What tool generates this? #Resolved
There was a problem hiding this comment.
This is generated by Rush itself #Resolved
|
Looks like we will need a corresponding change to all the build definitions in VSTS. Incidentally, why is install-run-rush needed now, do you know? Would the old approach no longer work? |
|
@natalieethell can you test and ensure that the precommit hooks still work? (Prettier in particular) #Resolved |
|
Looking at the commit log, it seems the change here (which will require corresponding changes to VSTS definitions) is to avoid a "phantom" node_modules folder - what does "phantom" here specifically refer to? #Resolved |
|
@cliffkoh from what I gathered from Pete, it seems like we're trying to avoid using anything in the root node_modules, since that is a phantom folder and Rush aims to protect against phantom dependencies. So, he put in the install run rush script instead. Is there another approach you can think of that avoids using rush from the root node_modules folder? #Resolved |
|
(If my understanding is correct) Rush's symlinking model aims to eliminate the phantom dependencies that could be introduced by the root node_modules folder, where phantom dependencies refer to broken imports or mismatched versions when someone else installs an OUFR package. With these, you could accidentally import a library that was missing from package.json. #Resolved |
|
Gotcha. That said, we still have a root node_modules folder... this change doesn't actually remove it... #Resolved |
|
Right! We're looking to remove it, but we're still looking to solve other issues that come up first. If there aren't too many and removing the node_modules folder isn't too involved, we'll do it. Otherwise, we can suppress the Rush warning. #Resolved |
|
(This PR still needs some work) #Resolved |
|
Before you (or anyone else) hit the merge button, please make sure to sync with myself and @kenotron so we can make sure to update the corresponding build definitions in VSTS (demo deploy and the release build). |
|
It looks like (jestjs/jest#6162 for reference) #Resolved |
natalieethell
requested review from
jakob101,
JasonGore,
joschect and
lorejoh12
as code owners
|
I checked all of the following commands using install-run-rush.js in package.json:
They seem to all work as expected. |
|
Thanks a bunch @natalieethell for getting this working! :-D |
|
Doing some final checks myself before merging. Do not merge yet. |
|
Looks good on my end! |
|
just waiting for passing build and ill merge |
|
Thanks Micah! Also just added notes to the wiki: https://github.com/OfficeDev/office-ui-fabric-react/wiki/Rush-5-Change-Notes |
|
🎉 Handy Links: |
|
So it seems we can no longer run global rush commands. |
|
@jordandrako I assume you have rush 5.x installed? |
|
Latest, so 5.2.1. I double checked by running |
If you encounter it again and can provide a repro, the Rush developers would be happy to help investigate. We definitely want this to be a good experience. :-) |
Pull request checklist
$ npm run changeDescription of changes
Upgrade OUFR to use Rush 5. This also removes the phantom node_modules folder from the root directory.
We will need a corresponding change to all the build definitions in VSTS.
Focus areas to test
I tested that the precommit hooks still work as expected.
Microsoft Reviewers: Open in CodeFlow