Skip to content

[release/3.1] Update SPA template JS dependencies #42060

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

Merged
merged 1 commit into from
Jun 13, 2022

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Jun 6, 2022

Update SPA template JS dependencies

Includes the routine update for SPA template dependencies. In addition, .js files importing react-router now instead import react-router-dom to accommodate a breaking change in a previous JS package update.

Description

This PR makes two changes:

  • Updates the JS dependency tree for the React, React+Redux, and Angular SPA templates.
  • Applies a fix for a bug introduced in a breaking change to a previous JS package update. The React templates had been mixing the use of react-router and react-router-dom, which only manifested itself as a real problem in the 3.1.25 SPA templates. The fix is to change all .js files importing react-router to instead import react-router-dom.

Fixes https://github.com/aspnet/AspNetCore-ManualTests/issues/1380

Customer Impact

Without the code change, customers will have to manually make the change themselves for all new SPA projects created from the templates in .NET Core 3.1. Without this fix, the error message that gets logged does not make the required code change obvious to customers.

Regression?

  • Yes
  • No

Regressed from .NET Core 3.1.24

Risk

  • High
  • Medium
  • Low

The fix simply adjusts our code to reference the correct package (react-router-dom instead of react-router). Other changes are the routine JS dependency updates.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@MackinnonBuck MackinnonBuck requested a review from Pilchie as a code owner June 6, 2022 21:15
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jun 6, 2022
@ghost ghost added this to the 3.1.x milestone Jun 6, 2022
@ghost
Copy link

ghost commented Jun 6, 2022

Hi @MackinnonBuck. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@MackinnonBuck MackinnonBuck added the tell-mode Indicates a PR which is being merged during tell-mode label Jun 6, 2022
@MackinnonBuck MackinnonBuck requested review from javiercn and wtgodbe June 6, 2022 21:16
@wtgodbe
Copy link
Member

wtgodbe commented Jun 6, 2022

Should the corresponding package.json files get updated too?

@MackinnonBuck
Copy link
Member Author

The package.json files didn't change, I just regenerated package-lock.json to update the dependency tree.

@MackinnonBuck
Copy link
Member Author

Let's wait to merge this - I still need to complete some validations.

@wtgodbe
Copy link
Member

wtgodbe commented Jun 9, 2022

@MackinnonBuck let me know when you want this merged (branches are open until EOD Monday)

@dougbu
Copy link
Contributor

dougbu commented Jun 10, 2022

Why is this a tell-mode change when it changes project template content❔ I'm probably missing something…

@MackinnonBuck
Copy link
Member Author

@dougbu Last month's dependency updates introduced a bug that is fixed by importing react-router-dom instead of react-router. Do you think this change is significant enough to bring this PR out of tell mode?

@dougbu
Copy link
Contributor

dougbu commented Jun 10, 2022

Do you think this change is significant enough to bring this PR out of tell mode?

I defer to @Pilchie

@Pilchie
Copy link
Member

Pilchie commented Jun 10, 2022

Let's split the difference - instead of just applying the label, let's actually send a mail to tactics with info about the change so that it's more broadly known, but not block on approval.

@MackinnonBuck
Copy link
Member Author

@wtgodbe I've completed my validations and the mail has been sent to tactics - can this be approved/merged now?

@dougbu
Copy link
Contributor

dougbu commented Jun 13, 2022

Since @javiercn is likely done for the day, I could approve but it would be better for someone at least slightly familiar w/ these two packages. Let me know if there's no one else around today

@dougbu dougbu merged commit 8b86293 into release/3.1 Jun 13, 2022
@dougbu dougbu deleted the mbuck/spa-template-dependency-update branch June 13, 2022 18:45
@dougbu
Copy link
Contributor

dougbu commented Jun 13, 2022

Thanks @javiercn

@dougbu dougbu modified the milestones: 3.1.x, 3.1.27 Jul 12, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants