Skip to content

refactor(schematics): detect package version from node modules #12535

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

Conversation

devversion
Copy link
Member

@devversion devversion commented Aug 5, 2018

No longer hard codes the Angular Material and Angular version name in the schematics.

  • For the CDK and Material version, the schematics will look for the according VERSION object in the node modules.

  • For the Angular version, we look for the @angular/core version in the package.json and try to use that. Falling back to the the 0.0.0-NG placeholder that specifies the required Angular version for Material and the CDK.

* No longer hard codes the Angular Material and Angular version name in the schematics. The schematics will look for the Material or CDK `VERSION` object in the node modules.
* For the Angular version, we look for the `@angular/core´ version in the `package.json` and try to use that. Falling back to the the `0.0.0-NG` placeholder that specifies the required Angular version for Material and the CDK.
@devversion devversion added pr: merge safe target: major This PR is targeted for the next major release labels Aug 5, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 5, 2018
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

I actually don't want to read the Angular version from the package.json. the way it is now, we have the flexibility to publish a range that is more broad than what we're installing. The required version should always be an explicit choice.

@devversion
Copy link
Member Author

devversion commented Aug 6, 2018

Are we talking just about the version for the @angular/animations package? From my perspective, it makes sense to align the @angular/animations version with the @angular/core version.

Otherwise the peer dependencies of @angular/animations can be wrong, and the developer would need to manually change the version afterwards, which kind of should be part of the schematic.

I'm fine just switching the 0.0.0-NG placeholder fallback to be the default for the @angular/animations, if you mean that.

@jelbourn
Copy link
Member

jelbourn commented Aug 6, 2018

I think I misunderstood your description. Would it be accurate to say

Use version of @angular/animations matching @angular/core instead of @angular/material

?

@devversion
Copy link
Member Author

devversion commented Aug 6, 2018

Sorry, the PR actually has two changes and I probably wasn't too clear in the description.

  • The @angular/animations version will match @angular/core version instead of being hard-coded to a loose version range. If the @angular/core version couldn't be determined --> we fall back to the minimum required Angular version for Material (0.0.0-NG placeholder)

  • The @angular/material and @angular/cdk package versions will be set to the version that comes with the schematics. Meaning that we just look for the currently installed (in the node modules) VERSION and write that to the package.json.

This is needed because when running: ng add @angular/material, the CLI just installs @angular/material (without saving in the package.json). So we need to save some version to the package.json.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

I should have just looked at the code instead of only reading the description

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Aug 6, 2018
@devversion
Copy link
Member Author

haha, my fault, sorry 😄 I felt like this was clear enough, but apparently it was too difficult to cover both two things in a short but clear way.

@josephperrott josephperrott merged commit 4ecde45 into angular:master Aug 7, 2018
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants