Skip to content

feat(compiler-cli): support transforming component style resources#41307

Closed
clydin wants to merge 4 commits intoangular:masterfrom
clydin:transform-resources
Closed

feat(compiler-cli): support transforming component style resources#41307
clydin wants to merge 4 commits intoangular:masterfrom
clydin:transform-resources

Conversation

@clydin
Copy link
Copy Markdown
Member

@clydin clydin commented Mar 22, 2021 •
edited
Loading

This change introduces a new hook on the ResourceHost interface named transformResource.
Resource transformation allows both external and inline resources to be transformed prior to
compilation by the AOT compiler. This provides support for tooling integrations to enable
features such as preprocessor support for inline styles.
This change also allows all resource transformation to be centralized in the new transformResource hook. Previously, the readResource hook was leveraged to both read and transform external styles.
Only style resources are currently supported. However, the infrastructure is in place to add
template support in the future.

  /**
   * Transform an inline or external resource asynchronously.
   * It is assumed the consumer of the corresponding `Program` will call
   * `loadNgStructureAsync()`. Using outside `loadNgStructureAsync()` will
   * cause a diagnostics error or an exception to be thrown.
   * Only style resources are currently supported.
   *
   * @param data The resource data to transform.
   * @param context Information regarding the resource such as the type and containing file.
   * @returns A promise of either the transformed resource data or null if no transformation occurs.
   */
  transformResource?
      (data: string, context: ResourceHostContext): Promise<TransformResourceResult|null>;
}

@clydin clydin added target: major This PR is targeted for the next major release area: compiler Issues related to `ngc`, Angular's template compiler labels Mar 22, 2021
@ngbot ngbot bot added this to the Backlog milestone Mar 22, 2021
@google-cla google-cla bot added the cla: yes label Mar 22, 2021
@clydin clydin force-pushed the transform-resources branch 5 times, most recently from dc43f5b to 303d10a Compare March 22, 2021 19:39
@clydin clydin marked this pull request as ready for review March 22, 2021 20:28
@pullapprove pullapprove bot requested a review from alxhub March 22, 2021 20:28
@clydin
feat(compiler-cli): support transforming component style resources
96015e3 
This change introduces a new hook on the `ResourceHost` interface named `transformResource`.
Resource transformation allows both external and inline resources to be transformed prior to
compilation by the AOT compiler. This provides support for tooling integrations to enable
features such as preprocessor support for inline styles.
Only style resources are currently supported. However, the infrastructure is in place to add
template support in the future.
@clydin clydin force-pushed the transform-resources branch from 303d10a to 96015e3 Compare March 24, 2021 13:49
@clydin clydin changed the title refactor(compiler-cli): support transforming component style resources feat(compiler-cli): support transforming component style resources Mar 24, 2021
@clydin clydin added the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 25, 2021
petebacondarwin
petebacondarwin reviewed Mar 25, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

One concern I have, which may be unfounded, with this feature is that resource pre-processing can only be done async. This means that compiling the application synchronously would produce a different result to compiling it asynchronously. I am not sure how common this would be a problem... does the CLI "always" run the pre-analyze async run? I think that unit testing via the TestBed would normally be sync?

@clydin
Copy link
Copy Markdown
Member Author

clydin commented Mar 26, 2021

The CLI and in turn the Angular Webpack plugin always execute asynchronously. Integration with the Webpack pipeline requires asynchronous compiler analysis. The current set of potential transformations are all asynchronous as well so at this point there would be no consumer for a synchronous variant. However, if such a requirement is later introduced, synchronous support could be added without any breaking API changes.

alxhub
alxhub approved these changes Mar 29, 2021
View reviewed changes
JoostK
JoostK requested changes Mar 29, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

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

I have some concerns about the preprocessing not being considered when loadNgStructureAsync is not used.

Additionally, I'd like to see integration-level tests using NgtscTestEnvironment, to actually run this through the whole pipeline.

@clydin clydin requested a review from JoostK March 30, 2021 00:29
JoostK
JoostK approved these changes Mar 30, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

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

The changes LGTM to land for v12. Would be good to expand our test coverage of (async) resource loading/processing in a followup PR.

@clydin clydin added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 30, 2021
@AndrewKushnir
Copy link
Copy Markdown
Contributor

AndrewKushnir commented Apr 2, 2021

Presubmit.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Apr 2, 2021
@atscott atscott closed this in 1de04b1 Apr 2, 2021
@clydin clydin deleted the transform-resources branch April 2, 2021 23:17
TeriGlover pushed a commit to TeriGlover/angular that referenced this pull request Apr 5, 2021
@clydin @TeriGlover
feat(compiler-cli): support transforming component style resources (a…
1ae0d27 
…ngular#41307)

This change introduces a new hook on the `ResourceHost` interface named `transformResource`.
Resource transformation allows both external and inline resources to be transformed prior to
compilation by the AOT compiler. This provides support for tooling integrations to enable
features such as preprocessor support for inline styles.
Only style resources are currently supported. However, the infrastructure is in place to add
template support in the future.

PR Close angular#41307
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented May 3, 2021

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 May 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JoostK JoostK JoostK approved these changes

@alxhub alxhub alxhub approved these changes

+1 more reviewer

@petebacondarwin petebacondarwin petebacondarwin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

5 participants

@clydin @AndrewKushnir @petebacondarwin @JoostK @alxhub