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
ngcc: implement ivy switch rendering #25534
Conversation
b858548
to
5038aa1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be other unrelated changes in this PR, but the __PRE_NGCC__
rewrite to __POST_NGCC__
looks good to me.
* @returns An array of variable declarations that match. | ||
*/ | ||
getSwitchableDeclarations(module: ts.Node): SwitchableVariableDeclaration[] { | ||
return findAll(module, isSwitchableVariableDeclaration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of right now this switch is in @angular/core
only. Should we short circuit this for perf reasons? Is there an argument to be made to enable such switch for others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to test this but I suspect that the bottleneck for performance is creating the TS compilation, which is necessary for the rest of the work. So I believe that parsing for these markers will make little performance difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest that we have a period of performance tuning once the functionality is in place. I think that there is potentially a lot of duplication of work, especially in the ngtsc resolver and the reflection hosts right now that would benefit from some caching strategies.
The other changes were to simplify the rendering tests, without which the changes to the tests would have got yet more complicated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments. LGTM otherwise 👍
export const POST_NGCC_MARKER = '__POST_NGCC__'; | ||
|
||
export type SwitchableVariableDeclaration = ts.VariableDeclaration & {initializer: ts.Identifier}; | ||
export function isSwitchableVariableDeclaration(node: ts.Node): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make this a method on the ReflectionHost
. Having both standalone functions and Class methods gets confusing (imo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have lots of type constraint functions in the ngcc and ngtsc code. They are all standalone.
*/ | ||
export function findAll<T>(node: ts.Node, test: (node: ts.Node) => node is ts.Node & T): T[] { | ||
const nodes: T[] = []; | ||
findAllWorker(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds more like a visitor than a worker 😁
} | ||
|
||
/** | ||
* Parse down the AST and capture a all the nodes that satisfy the test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a all --> all
@@ -82,6 +94,27 @@ export class A {}`); | |||
}); | |||
}); | |||
|
|||
describe('rewriteSwitchableDeclarations', () => { | |||
it('switch marked declaration initializers', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+should (?)
@@ -46,6 +46,18 @@ export class C {} | |||
C.decorators = [ | |||
{ type: Directive, args: [{ selector: '[c]' }] }, | |||
]; | |||
let compileNgModuleFactory = compileNgModuleFactory__PRE_NGCC__; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't mind a test with __PRE_NGCC__
at the beginning of the identifier 😁
const MARKER_FILE = { | ||
name: '/marker.js', | ||
contents: ` | ||
let compileNgModuleFactory = compileNgModuleFactory__PRE_NGCC__; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't mind a test with __PRE_NGCC__
at the beginning of the identifier 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
contents: ` | ||
const PROGRAM = { | ||
name: 'some/file.js', | ||
contents: ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a helper function to strip indentation makes the code look (and get folded) much better. I usually have:
const stripIndentation = (input: string): string => {
const lines = input.replace(/^ *\n/, '').replace(/\n *$/, '').split('\n');
const minIndentation = Math.min(...lines.
filter(l => !/^ *$/.test(l)).
map(l => /^ */.exec(l)![0].length));
const re = new RegExp(`^ {0,${minIndentation}}`);
return lines.map(l => l.replace(re, '')).join('\n');
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. This has been a worry for me for a while. I'll give this a go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the two replaces? Blank lines get stripped in the filter right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add that helper in a separate PR (since it would hit a lot of the tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM.
why the two replaces? Blank lines get stripped in the filter right?
The two replace
remove leading/trailing empty/whitespace-only lines from the output. The filter
is only used to ignore whitespace-only lines for computing the min indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RIght!
}); | ||
}); | ||
|
||
describe('rewriteSwitchableDeclarations', () => { | ||
it('switch marked declaration initializers', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+should (?)
Can a Googler run the presubmit please? |
6d7e15a
to
059a9e1
Compare
Rebased on master. Hoping @alxhub can get this through G3 etc. |
You can preview 059a9e1 at https://pr25534-059a9e1.ngbuilds.io/. |
059a9e1
to
ae1b36f
Compare
You can preview ae1b36f at https://pr25534-ae1b36f.ngbuilds.io/. |
@@ -68,7 +71,7 @@ describe('Renderer', () => { | |||
] | |||
}); | |||
const RENDERED_CONTENTS = | |||
`\n// REMOVE DECORATORS\n\n// ADD IMPORTS\n\n// ADD CONSTANTS\n\n// ADD DEFINITIONS\n` + | |||
`\n// REWRITTEN DECLARATIONS\n\n// REMOVE DECORATORS\n\n// ADD IMPORTS\n\n// ADD CONSTANTS\n\n// ADD DEFINITIONS\n` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm becoming less of a fan of this approach of testing. It's really hard to look at this test and figure out whether it's correct.
Can we start thinking of a way to make this more straightforward? It doesn't have to happen in this PR.
* @returns An array of variable declarations that match. | ||
*/ | ||
getSwitchableDeclarations(module: ts.Node): SwitchableVariableDeclaration[] { | ||
return findAll(module, isSwitchableVariableDeclaration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should implement a performance optimization where we check module.getText()
for PRE_NGCC_MARER
via regex or .indexOf
. Only if the test is positive should we walk the AST to actually find them.
This method will be used to find all the places where the "ivy switch" will occur. See angular#25238
This supports the "ngcc ivy switch" specified in angular#25238.
Also incorporates a refactoring of the tests to make them less fragile.
Only parse the AST for ngcc ivy switch constants if the marker is not found in the module text.
ae1b36f
to
29457cc
Compare
You can preview 29457cc at https://pr25534-29457cc.ngbuilds.io/. |
@alxhub can you help get the G3 presubmit green so that we can merge this? |
Also incorporates a refactoring of the tests to make them less fragile. PR Close #25534
Only parse the AST for ngcc ivy switch constants if the marker is not found in the module text. PR Close #25534
…angular#25534) This method will be used to find all the places where the "ivy switch" will occur. See angular#25238 PR Close angular#25534
) This supports the "ngcc ivy switch" specified in angular#25238. PR Close angular#25534
Also incorporates a refactoring of the tests to make them less fragile. PR Close angular#25534
Only parse the AST for ngcc ivy switch constants if the marker is not found in the module text. PR Close angular#25534
…angular#25534) This method will be used to find all the places where the "ivy switch" will occur. See angular#25238 PR Close angular#25534
) This supports the "ngcc ivy switch" specified in angular#25238. PR Close angular#25534
Also incorporates a refactoring of the tests to make them less fragile. PR Close angular#25534
Only parse the AST for ngcc ivy switch constants if the marker is not found in the module text. PR Close angular#25534
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This is related to #25238