Skip to content

feat(@angular-devkit/build-angular): deprecate scripts and styles `la… #14955

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 2 commits into from
Jul 2, 2019
Merged

feat(@angular-devkit/build-angular): deprecate scripts and styles `la… #14955

merged 2 commits into from
Jul 2, 2019

Conversation

alan-agius4
Copy link
Collaborator

…zy option in favor ofinject`

The lazy option inside the script and style option is confusing as this option doesn't lazy load a bundle but rather it doesn't inject/reference the script in the HTML. While this option is an enabler for lazy loading, the users will still need to handle on how how this bundle will be lazy loaded. There are also potential use cases beyond lazy loading for the option.

Closes #14814

@alan-agius4 alan-agius4 requested a review from filipesilva July 1, 2019 11:37
@alan-agius4 alan-agius4 added the target: major This PR is targeted for the next major release label Jul 1, 2019
filipesilva
filipesilva previously approved these changes Jul 1, 2019
Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

LGTM in general. We'll need a migration for this in 9 though.

Unsure of how we should process that work. If we just add it now, it might not run as people update project versions manually. I guess we need a PR for it that we just merge during RC?

@alan-agius4
Copy link
Collaborator Author

What I had in mind is to add such things as a future migration. Ie, it will already be inside the code and unit tested but it won't run when doing ng update, as the target will be version 9.0.0-beta.0.

What do you reckon?

@filipesilva
Copy link
Contributor

I think that's a pretty good approach.

@alan-agius4
Copy link
Collaborator Author

Will do the update schematic in a separate PR in the coming days

@filipesilva
Copy link
Contributor

Is it important that it's in a separate PR? Having everything in a single PR would generally be better.

…zy` option in favor of`inject`

The lazy option inside the script and style option is confusing as this option doesn't lazy load a bundle but rather it doesn't inject/reference the script in the HTML. While this option is an enabler for lazy loading, the users will still need to handle on how how this bundle will be lazy loaded. There are also potential use cases beyond lazy loading for the option.

Closes #14814
@alan-agius4 alan-agius4 requested a review from filipesilva July 2, 2019 10:39
@alan-agius4 alan-agius4 dismissed filipesilva’s stale review July 2, 2019 10:39

Pushed migration script. PTAL

Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

@vikerman vikerman merged commit 040e3ea into angular:master Jul 2, 2019
@alan-agius4 alan-agius4 deleted the feat-inject branch July 2, 2019 18:47
@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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate styles and script lazy option in favor of a more self descriptive option
4 participants