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

feat: autoinstall Angular v17 #5367

Merged
merged 7 commits into from
Nov 8, 2023
Merged

feat: autoinstall Angular v17 #5367

merged 7 commits into from
Nov 8, 2023

Conversation

Skn0tt
Copy link
Contributor

@Skn0tt Skn0tt commented Nov 6, 2023

In netlify/angular-runtime#67, we're working on improved Angular support for v17+. This PR adds auto-install for the package.
To be merged after netlify/angular-runtime#67 is done + released.

@Skn0tt Skn0tt self-assigned this Nov 6, 2023
@Skn0tt Skn0tt requested review from a team as code owners November 6, 2023 17:23
"plugins": [
{
"packageName": "@netlify/angular-runtime",
"condition": { "minNodeVersion": "17.0.0" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this what you meant to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, it isn't! I meant to do "from angular v17 on, use this". gonna look at how to do that. good catch!´

Copy link
Contributor

Choose a reason for hiding this comment

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

It's changed since I last worked on it, so I'm afraid I can't help there!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented minFrameworkVersion in e6298e9

@Skn0tt Skn0tt changed the title fix: autoinstall Angular v17 feat: autoinstall Angular v17 + introduce minFrameworkVersion Nov 7, 2023
@ascorbic
Copy link
Contributor

ascorbic commented Nov 7, 2023

I'm pretty sure the versioning must already be implemented elsewhere though. We're installing different versions of the Next.js runtime according to which version of next it is, so that logic must be somewhere! It used to be in plugins.json, but I think it's in the CMS now

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Nov 8, 2023

We have versioning implemented + configured via plugins.json, that's true - but it's for selecting different versions of the plugin depending on the framework version. This one is slightly different: It's about auto-installing or not, depending on the framework version. I don't think we've implemented that anywhere so far.

@Skn0tt Skn0tt changed the title feat: autoinstall Angular v17 + introduce minFrameworkVersion feat: autoinstall Angular v17 Nov 8, 2023
@Skn0tt
Copy link
Contributor Author

Skn0tt commented Nov 8, 2023

Lukas reminded me that we're not using framework-info anymore. Good news, because build-info makes this change a lot simpler!

@Skn0tt Skn0tt requested a review from ascorbic November 8, 2023 11:42
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

That's better! The new system is so much more flexible. I'm approving this but I reckon I'd wait for a review from Lukas or someone else who knows the detection code better before merging

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Nov 8, 2023

I'm pretty confident in the approach based on tests. Will merge now in the interest of time, let's address any feedback lukas might have in a follow-up.

@Skn0tt Skn0tt enabled auto-merge (squash) November 8, 2023 13:46
Copy link
Contributor

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

LGTM!

@Skn0tt Skn0tt merged commit 6a75332 into main Nov 8, 2023
34 checks passed
@Skn0tt Skn0tt deleted the detect-angular-v17 branch November 8, 2023 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants