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

Downgrade rush because it is causing commandline args not to appear in npm run scripts #9445

Merged
merged 1 commit into from
Jun 13, 2019

Conversation

kenotron
Copy link
Member

@kenotron kenotron commented Jun 13, 2019

Ever since we upgraded rush to 5.9.0, the --production (or indeed any flags from rush commands) are not passed down to the npm run scripts. So this makes us not build any minified or AMD builds. I'm downgrading to what we had before to keep building AMD bundles

Microsoft Reviewers: Open in CodeFlow

@msft-github-bot
Copy link
Contributor

Component perf results:

Scenario Target branch avg total (ms) PR avg total (ms) Target branch avg per item (ms) PR avg per item (ms) Is significant change Is regression
ScenarioDefaultButton 42.673 42.363 0.427 0.424 false false

@size-auditor
Copy link

size-auditor bot commented Jun 13, 2019

Size Auditor did not detect a change in bundle size for any component!

@KevinTCoughlin
Copy link
Member

KevinTCoughlin commented Jun 13, 2019

Fixed in microsoft/rushstack#1334?

https://www.npmjs.com/package/@microsoft/rush/v/5.9.1

cc: @iclanton

Copy link
Member

@KevinTCoughlin KevinTCoughlin left a comment

Choose a reason for hiding this comment

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

Ok with downgrade to mitigate, but you might consider upgrading to 5.9.1 instead.

@kenotron
Copy link
Member Author

Yeah, mitigate for now. 5.9.0 has a behavioral change that we need to consider before upgrading really:

(BEHAVIOR CHANGE) Fix an issue where CI jobs could succeed even if a task reported warnings to stderr; if your build fails due to warnings after upgrading, please see microsoft/rushstack#1329

Looks like we'd probably upgrade to pnpm 3 + rush >5.7.0 if we want to pursue this path. I think we should think through this a bit more before just taking latest.

@kenotron kenotron merged commit fa84fcc into microsoft:master Jun 13, 2019
@kenotron kenotron deleted the rush-down branch June 13, 2019 17:23
@kenotron
Copy link
Member Author

coordinating with @jdhuntington who will create another PR to bump everything for prod release

@iclanton
Copy link
Member

FYI, 5.8.0 doesn't have the behavioral change, but does have additional fixes.

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants