-
Notifications
You must be signed in to change notification settings - Fork 787
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
feat(compiler): Stencil decorator import aliasing #5161
Conversation
this commit creates a map of original import names to aliased import names. This will be used to get the aliased name of a decorator when parsing Stencil decorators to static component metadata
This commit creates an `ImportAliasMap` class that extends the `Map` class. This makes it easy to instantiate, reference the underlying type, and clearly override the default getter
The commit passes the aliased import name from the `ImportAliasMap` to each respective decorator transformer Fixes: #3137 STENCIL-263
|
Path | Error Count |
---|---|
src/dev-server/index.ts | 37 |
src/mock-doc/serialize-node.ts | 36 |
src/dev-server/server-process.ts | 32 |
src/compiler/output-targets/dist-lazy/generate-lazy-module.ts | 22 |
src/compiler/prerender/prerender-main.ts | 22 |
src/testing/puppeteer/puppeteer-element.ts | 22 |
src/runtime/client-hydrate.ts | 20 |
src/screenshot/connector-base.ts | 19 |
src/runtime/vdom/vdom-render.ts | 18 |
src/compiler/config/test/validate-paths.spec.ts | 16 |
src/dev-server/request-handler.ts | 15 |
src/compiler/prerender/prerender-optimize.ts | 14 |
src/compiler/sys/stencil-sys.ts | 14 |
src/compiler/transpile/transpile-module.ts | 14 |
src/runtime/vdom/vdom-annotations.ts | 14 |
src/sys/node/node-sys.ts | 14 |
src/compiler/prerender/prerender-queue.ts | 13 |
src/compiler/sys/in-memory-fs.ts | 13 |
src/runtime/connected-callback.ts | 13 |
src/runtime/set-value.ts | 13 |
Our most common errors
Typescript Error Code | Count |
---|---|
TS2345 | 376 |
TS2322 | 373 |
TS18048 | 286 |
TS18047 | 102 |
TS2722 | 37 |
TS2532 | 30 |
TS2531 | 23 |
TS2454 | 14 |
TS2352 | 12 |
TS2790 | 10 |
TS2769 | 8 |
TS2538 | 8 |
TS2344 | 6 |
TS2416 | 6 |
TS2493 | 3 |
TS2488 | 2 |
TS18046 | 2 |
TS2684 | 1 |
TS2430 | 1 |
Unused exports report
There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!
Unused exports
File | Line | Identifier |
---|---|---|
src/runtime/bootstrap-lazy.ts | 21 | setNonce |
src/screenshot/screenshot-fs.ts | 18 | readScreenshotData |
src/testing/testing-utils.ts | 198 | withSilentWarn |
src/utils/index.ts | 145 | CUSTOM |
src/utils/index.ts | 269 | normalize |
src/utils/index.ts | 7 | escapeRegExpSpecialCharacters |
src/compiler/app-core/app-data.ts | 25 | BUILD |
src/compiler/app-core/app-data.ts | 115 | Env |
src/compiler/app-core/app-data.ts | 117 | NAMESPACE |
src/compiler/fs-watch/fs-watch-rebuild.ts | 123 | updateCacheFromRebuild |
src/compiler/types/validate-primary-package-output-target.ts | 61 | satisfies |
src/compiler/types/validate-primary-package-output-target.ts | 61 | Record |
src/testing/puppeteer/puppeteer-declarations.ts | 485 | WaitForEventOptions |
src/compiler/sys/fetch/write-fetch-success.ts | 7 | writeFetchSuccessSync |
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.
One non blocking question, rest LGTM 👍
} | ||
|
||
override get(key: StencilDecorator): string { | ||
return super.get(key) ?? key; |
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.
Is there a valid use case to ask for a decorator key we don't know?
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 isn't for the case of a decorator we don't know, but for a decorator that wasn't used in the component. If we don't override this, when we pass decorator names into the functions responsible for transforming them, we'd need to do importAliasMap.get('Watch') ?? 'Watch'
. Since if the component didn't have a watch function, the map would not have a key for Watch
and would return undefined
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 could, alternatively, change the class so that it always has all the Stencil decorators and we just change the value for any aliased keys, works the same.
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.
Thanks for clarifying. This makes sense to me and can stay as is.
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.
@tanner-reits LGTM, thanks! Can you do me a favor and open a complimentary PR to this one (and link it in the PR Description) to update the verbiage in our documentation regarding type conflicts? Specifically, I'm looking at https://github.com/ionic-team/stencil-site/blob/e16218ad879ad1d7aedd5365abf2b764e5357329/docs/components/events.md?plain=1#L85-L90
What is the current behavior?
It is not possible to use aliases (i.e. "as") when importing Stencil decorators because of the compiler's meta-to-static conversions of all Stencil decorators.
GitHub Issue Number: #3137
What is the new behavior?
Import aliasing is now possible as we map aliased imports back to their original decorator name. The aliased name is then passed to all the transformation functions.
Does this introduce a breaking change?
Testing
Manual testing
Automated testing
Added e2e test that tests a subset of Stencil decorators with aliased import names.
Other information
PR to update relevant docs: ionic-team/stencil-site#1292