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

feat(compiler): Stencil decorator import aliasing #5161

Merged
merged 6 commits into from
Dec 13, 2023

Conversation

tanner-reits
Copy link
Member

@tanner-reits tanner-reits commented Dec 12, 2023

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?

  • Yes
  • No

Testing

Manual testing

  • This can be easily tested in a Stencil component starter by aliasing any Stencil decorator used in a component.
  • Also manually tested in a project using a CE build

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

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
Copy link
Contributor

github-actions bot commented Dec 12, 2023

--strictNullChecks error report

Typechecking with --strictNullChecks resulted in 1300 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files
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

@tanner-reits tanner-reits marked this pull request as ready for review December 12, 2023 16:52
@tanner-reits tanner-reits requested a review from a team as a code owner December 12, 2023 16:52
Copy link
Member

@christian-bromann christian-bromann left a 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;
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

@rwaskiewicz rwaskiewicz left a 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

@tanner-reits
Copy link
Member Author

@tanner-reits tanner-reits added this pull request to the merge queue Dec 13, 2023
Merged via the queue into main with commit 97dcb45 Dec 13, 2023
120 checks passed
@tanner-reits tanner-reits deleted the treits/feat/decorator-import-aliasing branch December 13, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants