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

Initial implementation of Standalone Components #44812

Closed
wants to merge 3 commits into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Jan 25, 2022

No description provided.

@alxhub alxhub force-pushed the ngtsc/sac/impl branch 3 times, most recently from d673adf to 171c988 Compare January 25, 2022 02:37
@alxhub alxhub force-pushed the ngtsc/sac/impl branch 3 times, most recently from 5d67801 to d6ce33c Compare January 25, 2022 22:01
@jessicajaniuk jessicajaniuk added the area: core Issues related to the framework runtime label Jan 27, 2022
@ngbot ngbot bot added this to the Backlog milestone Jan 27, 2022
import {ComponentScopeReader} from '../../../scope';
import {ComponentScopeReader, DtsModuleScopeResolver, ExportScope, LocalModuleScopeRegistry} from '../../../scope';

import {ComponentAnalysisData} from './metadata';


export interface ScopeTemplateResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it makes sense to have two separate interfaces at this point? ngModule is always null for standalone components and diagnostics are always empty for components declared in modules.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would make things more complicated, honesty.

Having multiple interfaces is only useful if there's a benefit to be gained by distinguishing between them. For consumers of ScopeTemplateResult, there is no significant benefit to be gained from understanding, for example, that diagnostics are only present if ngModule is not. Adding the checks required to narrow the type appropriately to collect the diagnostics when present would be a net negative in terms of complexity.

Comment on lines 57 to 81
throw createValueHasWrongTypeError(
ref.getOriginForDiagnostics(expr), ref,
`'imports' must be an array of components, directives, pipes, or NgModules`);
}
} else {
throw createValueHasWrongTypeError(
expr, imports,
`'imports' must be an array of components, directives, pipes, or NgModules`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These errors will mean that the language service won't give any template information for components with broken imports. Are we alright with that or should we maybe attempt to fail more gracefully and still process as much as possible? I think this is probably fine, just wanted to bring it up -- there are a number of other errors in the meta that do this as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that we should probably move away from throwing FatalDiagnosticError. I've refactored this function to return diagnostics instead, which should allow some information to be extracted in the LS regardless of errors in imports.

@pullapprove pullapprove bot requested a review from atscott January 27, 2022 22:03
Comment on lines 64 to 73
/**
* Raised when a type in the `imports` of a component is a directive or pipe, but is not
* standalone.
*/
COMPONENT_IMPORT_NOT_STANDALONE = 2011,

/**
* Raised when a type in the `imports` of a component is not a directive, pipe, or NgModule.
*/
COMPONENT_UNKNOWN_IMPORT = 2011,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these two shouldn't have the same code, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

@pullapprove pullapprove bot requested a review from atscott January 27, 2022 22:04
@trumbitta
Copy link

image

"No description provided"

This is a long awaited, much dreamt-of, table-turning, etc. etc., feature. I'd love to see an extensive description of what it is and what it means for Angular, in the PR.

@bampakoa
Copy link
Contributor

bampakoa commented Feb 1, 2022

@trumbitta check out the initial RFC #43784 I hope that helps 🙂

@alxhub alxhub force-pushed the ngtsc/sac/impl branch 2 times, most recently from bba2b04 to fe776a7 Compare February 1, 2022 22:57
…ages

Previously each `DecoratorHandler` in the compiler was stored in a single file
in the 'annotations' package. The `ComponentDecoratorHandler` in particular was
several thousand lines long.

Prior to implementing the new standalone functionality for components, this
commit refactors 'annotations' to split these large files into their own build
targets with multiple separate files. This should make the implementation of
standalone significantly cleaner.
In preparation for standalone components, this commit moves the logic which
determines the potential set of components/directives/pipes in a template into
a separate function. This is a simple but crucial refactoring that breaks the
assumption that all template scopes come from NgModules.
This commit implements the first phase of standalone components in the Angular
compiler. This mainly includes the scoping rules for standalone components
(`@Component({imports})`).

Significant functionality from the design is _not_ implemented by this PR,
including:

* imports of standalone components into NgModules.
* the provider aspect of standalone components

Future commits will address these issues, as we proceed with the design of
this feature.
@alxhub alxhub added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Feb 2, 2022
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@pullapprove pullapprove bot requested a review from jelbourn February 3, 2022 01:08
Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

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

Approving for the new error codes in the .md file.

reviewed-for: public-api

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Looks great, thanks Alex! 👍

Reviewed-for: public-api

@alxhub alxhub added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 3, 2022
@alxhub alxhub removed the request for review from jelbourn February 3, 2022 02:04
@dylhunn
Copy link
Contributor

dylhunn commented Feb 3, 2022

This PR was merged into the repository by commit 0072eb4.

@dylhunn dylhunn closed this in cc0d73d Feb 3, 2022
dylhunn pushed a commit that referenced this pull request Feb 3, 2022
#44812)

In preparation for standalone components, this commit moves the logic which
determines the potential set of components/directives/pipes in a template into
a separate function. This is a simple but crucial refactoring that breaks the
assumption that all template scopes come from NgModules.

PR Close #44812
dylhunn pushed a commit that referenced this pull request Feb 3, 2022
…44812)

This commit implements the first phase of standalone components in the Angular
compiler. This mainly includes the scoping rules for standalone components
(`@Component({imports})`).

Significant functionality from the design is _not_ implemented by this PR,
including:

* imports of standalone components into NgModules.
* the provider aspect of standalone components

Future commits will address these issues, as we proceed with the design of
this feature.

PR Close #44812
const {param, index, reason} = error;
let chainMessage: string|undefined = undefined;
let hints: ts.DiagnosticRelatedInformation[]|undefined = undefined;
switch (reason.kind) {
Copy link

@kenobi-io kenobi-io Feb 7, 2022

Choose a reason for hiding this comment

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

switch case is bad practics. Maybe using 'strategy' approach, for exapmle:

const kindHandlers = new Map().set(ValueUnavailableKind.UNSUPPORTED, () => {
chainMessage ='Consider using the @Inject decorator to specify an injection token.';
hints = [makeRelatedInformation( reason.typeNode, 'This type is not supported as injection token.' ), ]; }).set(...).set(...);
const handler = kindHandlers.get(reason.kind);
handler && handler.call(this, option);

@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 Mar 10, 2022
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this pull request Apr 8, 2022
…ages (angular#44812)

Previously each `DecoratorHandler` in the compiler was stored in a single file
in the 'annotations' package. The `ComponentDecoratorHandler` in particular was
several thousand lines long.

Prior to implementing the new standalone functionality for components, this
commit refactors 'annotations' to split these large files into their own build
targets with multiple separate files. This should make the implementation of
standalone significantly cleaner.

PR Close angular#44812
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this pull request Apr 8, 2022
angular#44812)

In preparation for standalone components, this commit moves the logic which
determines the potential set of components/directives/pipes in a template into
a separate function. This is a simple but crucial refactoring that breaks the
assumption that all template scopes come from NgModules.

PR Close angular#44812
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this pull request Apr 8, 2022
…ngular#44812)

This commit implements the first phase of standalone components in the Angular
compiler. This mainly includes the scoping rules for standalone components
(`@Component({imports})`).

Significant functionality from the design is _not_ implemented by this PR,
including:

* imports of standalone components into NgModules.
* the provider aspect of standalone components

Future commits will address these issues, as we proceed with the design of
this feature.

PR Close angular#44812
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 area: core Issues related to the framework runtime target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants