chore(angular): test schematics and code-splitting - #31401
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Glad to see this, it's the test I was after on the exports PR. Two big things I noticed though, the export type move breaks IonicSafeString at runtime, and neither new test fails yet on what it's protecting. There's some other things worth looking at too though, and some nits.
| 'ion-select-popover', | ||
| 'ion-slides', | ||
| ]; | ||
| const KNOWN_NON_CORE_ION_COMPONENTS = [ |
There was a problem hiding this comment.
I think this is the FW-7695 part of the PR, but I can't find the rest of it. That ticket asks for the standalone e2e pages to move to per-component imports and for this script to check every export subpath resolves from the barrel too, and the standalone pages are all still on the barrel.
The code-split app does typecheck the entry points for the seven components it uses, which is great, but it's not quite the same coverage. Is FW-7695 meant to be a follow-up?
Nit: double space before the =, and the array's missing a semicolon and trailing comma compared to the one above it. Nothing will catch those since scripts is ignored by eslint and prettier.
There was a problem hiding this comment.
I wanted to use the existing test app instead of making a new code-split test, but especially with the lazy-loaded side of the app included it's way too big and complicated.
Since your last review I did add a couple steps to test-angular-e2e that runs the import migration script and re-run the tests.
There was a problem hiding this comment.
Maybe the migration script should error if anything that's not a type import didn't get replaced?
There was a problem hiding this comment.
Yeah, though I'd scope it narrower than anything unreplaced, since unmapped is legitimately non-empty for symbols with no dedicated entry point. The case that matters is rewriting nothing at all.
Ran into this by accident: pointed the codemod at a build from before this PR's seven new subpaths, and because ionic-route-strategy wasn't among them it left one bare barrel import in main.ts, exited 0, and every Ionic component ended up in main.js. The split was gone and the script reported success. A flag that exits non-zero when zero statements were rewritten, passed in the workflow step, would catch that along with map drift and running from the wrong directory.
Those seven new subpaths turn out to be load-bearing for the whole test, which I hadn't realised.
Separately, nothing in the repo says how to run any of this. The test README covers the build.sh flow and this fixture deliberately sits outside it, so someone hitting a red test-angular-package has nowhere to look.
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Getting close! The job split and the new build step in the schematics test are both good additions.
Two bigger ones this round. The codemod corrupts any import that has a comment in it, and the code-split assertion still passes when a component ends up in main.js. I built the fixture to check both. There are replies on the older threads too, including a couple where you were right and I wasn't.
3 participants
Issue number: internal
What is the new behavior?
package.json.Does this introduce a breaking change?