feat(core): add undecorated classes migration schematic #31650
Conversation
devversion
added
action: review
devversion
changed the title
devversion
force-pushed
the
wip-base-classes
branch
2 times, most recently
from
99acb68 to
9631211
Compare
|
We now have a MapReduce over g3 that brings up an ngc |
|
I wired it up directly in tsunami, so you can remove any google3-specific or tslint bits from this PR. An ngTsunami analyzer is a program which gets a tsconfig path as its only argument. To start with I just want to find all the violations in google3, like tslint without It's not working for all inputs though, there's a crash: The spread operator appears in this code, hope this is enough to repro: the There's a second case of the same error, code looks like In both cases I think ngUpgrade is in use. I'll re-run the MR with some extra error-handling so we can find out if these are all the crashes. |
|
Also FWIW, 3 out of 3337 shards of g3 experienced the crash, so we're not losing much data due to this currently. |
|
okay with those three shards excluded, I have the code locations violating the check |
|
It triggers in this case which seems wrong |
|
@alexeagle Thanks for spending time on this! Finding violations first sounds reasonable to me.
The returned transform failures are only for instances where the migration could not be performed automatically. Successful replacements are stored in the update recorders. So right now I think that your MR test-run just showed cases where the migration failed to perform.
Hmm. The transform should actually never create failures for TS nodes outside of program source files. What are the violation messages for such instances?. |
devversion
force-pushed
the
wip-base-classes
branch
from
9631211 to
ba0c418
Compare
|
@alexeagle Removed the tslint driver bits in favor of the tsunami driver. Also I fixed the For case 1 where the pipe is decorated but inherits the constructor from undecorated pipes, we'd need to figure out what to do. cc. @alxhub. do we need an abstract |
devversion
force-pushed
the
wip-base-classes
branch
from
ba0c418 to
9116aeb
Compare
|
Probably shouldn't add a class comment if there is already one: |
|
I think we should always omit the leading/trailing comment for the copied decorator. Comments that are associated with a decorator are most of the time not intended to target the decorator, but rather the class. |
devversion
force-pushed
the
wip-base-classes
branch
4 times, most recently
from
cf2a823 to
7e8391f
Compare
packages/core/schematics/migrations/undecorated-classes/create_ngc_program.ts
Outdated
Show resolved
Hide resolved
packages/core/schematics/test/undecorated_classes_migration_spec.ts
Outdated
Show resolved
Hide resolved
kara
added
the
action: cleanup
devversion
added
action: cleanup
|
The PR is currently blocked on a components build issue that has been unveiled by these changes. I'll rebase this PR on top of angular/components#16758 once it landed. |
devversion
force-pushed
the
wip-base-classes
branch
from
bb1371d to
9277c82
Compare
devversion
added
action: presubmit
devversion
force-pushed
the
wip-base-classes
branch
2 times, most recently
from
3ad8430 to
e3c686a
Compare
kara
added
action: merge
This comment has been minimized.
This comment has been minimized.
kara
added
action: cleanup
|
Caretaker note (to self): needs a G3 patch |
Introduces a new migration schematic that follows the given migration plan: https://hackmd.io/@alx/S1XKqMZeS. First case: The schematic detects decorated directives which inherit a constructor. The migration ensures that all base classes until the class with the explicit constructor are properly decorated with "@directive()" or "@component". In case one of these classes is not decorated, the schematic adds the abstract "@directive()" decorator automatically. Second case: The schematic detects undecorated declarations and copies the inherited "@directive()", "@component" or "@pipe" decorator to the undecorated derived class. This involves non-trivial import rewriting, identifier aliasing and AOT metadata serializing (as decorators are not always part of source files)
Updates the `material-unit-tests` job to the latest commit on the components repository. 097f433. This commit ensures that the postinstall script does not run NGC on schematic code from `@angular/core`. Running NGC on the generated schematic code can cause unexpected issues as some migrations import types directly from `@angular/compiler-cli` while the entry-point is not usable in all cases. See: angular#29220.
devversion
force-pushed
the
wip-base-classes
branch
from
e3c686a to
7166d1e
Compare
devversion
removed
the
action: cleanup
kara
added
the
merge: caretaker note
|
merge-assistance: global |
Updates the `material-unit-tests` job to the latest commit on the components repository. 097f433. This commit ensures that the postinstall script does not run NGC on schematic code from `@angular/core`. Running NGC on the generated schematic code can cause unexpected issues as some migrations import types directly from `@angular/compiler-cli` while the entry-point is not usable in all cases. See: #29220. PR Close #31650
Introduces a new migration schematic that follows the given migration plan: https://hackmd.io/@alx/S1XKqMZeS. First case: The schematic detects decorated directives which inherit a constructor. The migration ensures that all base classes until the class with the explicit constructor are properly decorated with "@directive()" or "@component". In case one of these classes is not decorated, the schematic adds the abstract "@directive()" decorator automatically. Second case: The schematic detects undecorated declarations and copies the inherited "@directive()", "@component" or "@pipe" decorator to the undecorated derived class. This involves non-trivial import rewriting, identifier aliasing and AOT metadata serializing (as decorators are not always part of source files) PR Close angular#31650
Updates the `material-unit-tests` job to the latest commit on the components repository. 097f433. This commit ensures that the postinstall script does not run NGC on schematic code from `@angular/core`. Running NGC on the generated schematic code can cause unexpected issues as some migrations import types directly from `@angular/compiler-cli` while the entry-point is not usable in all cases. See: angular#29220. PR Close angular#31650
|
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. |
Regarding google3 tslint driver. I'm still worried about performance here. TSLint is generally pretty slow, doesn't have a global analysis phase etc. read more why tslint is bad for replacements here (bullet points).
It's especially bad now because the rule for this migration re-creates another program for each source file! (significant memory pressure; so much overhead).. We need to create another program because we need the
AngularCompilerProgramto read/parse and simplify AOT metadata and summaries.We could play with caching the program to reduce memory pressure.. but ideally we don't run the migration with tslint in google3.. or we just wait and hope that it goes through.. It might be worth exploring the upgrade tool we built for the Angular CDK migrations (which solves most of the issues above).