fix(compiler): add delegatesFocus to custom elements targets #3117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rwaskiewicz
commented
Oct 21, 2021
| import { transpileModule } from './transpile'; | ||
| import { nativeComponentTransform } from '../component-native/tranform-to-native-component'; | ||
|
|
||
| describe('nativeComponentTransform', () => { |
There was a problem hiding this comment.
I don't love how this is implemented, but I mirrored the how lazy components are tested here for simplicity's sake
rwaskiewicz
commented
Oct 21, 2021
| @@ -19,7 +19,7 @@ | |||
| "karma.prod": "npm run build.sibling && npm run build.invisible-prehydration && npm run build.app && npm run karma.webpack && npm run build.prerender && npm run karma", | |||
| "karma.ie": "karma start karma.config.js --browsers=IE --single-run=false", | |||
| "karma.edge": "karma start karma.config.js --browsers=Edge --single-run=false", | |||
| "karma.webpack": "webpack-cli --config test-app/esm-webpack/webpack.config.js && webpack-cli --config test-app/custom-elements-output-webpack/webpack.config.js && webpack-cli --config test-app/custom-elements-output-tag-class-different/webpack.config.js", | |||
| "karma.webpack": "webpack-cli --config test-app/esm-webpack/webpack.config.js && webpack-cli --config test-app/custom-elements-output-webpack/webpack.config.js && webpack-cli --config test-app/custom-elements-output-tag-class-different/webpack.config.js && webpack-cli --config test-app/custom-elements-delegates-focus/webpack.config.js", | |||
There was a problem hiding this comment.
I'm going to be working on making this more maintainable this sprint with STENCIL-48. The long term plan is not to tack on to this command for every custom-elements test
this commit allows `delegatesFocus` to be properly applied to components generated using the following output targets: - dist-custom-elements - dist-custom-elements-bundle the generation of the `attachShadow` call is moved from a standalone function to being attached to the prototype of the custom element when we proxy it. the reason for this is that we need the component metadata to to determine whether or not each individual component should have delegateFocus enabled or not. this led to the removal of the original standalone attachShadow function. I do not consider this to be a breaking change, as we don't publicly state our runtime APIs are available for general consumption. this change also led to the transition from using ts.create*() calls to ts.factory.create*() calls for nativeAttachShadowStatement, which is the general direction I'd like to take such calls, since the former is now deprecated STENCIL-90: "dist-custom-elements-bundle" does not set delegatesFocus when attaching shadow
rwaskiewicz
force-pushed
the
stencil90-delegate-focus
branch
from
83129fd to
d035edb
Compare
rwaskiewicz
commented
Oct 21, 2021
| const elm: Element = app.querySelector('custom-elements-delegates-focus'); | ||
|
|
||
| expect(elm.shadowRoot).toBeDefined(); | ||
| // as of TypeScript 4.3, `delegatesFocus` does not exist on the `shadowRoot` object |
There was a problem hiding this comment.
This may not be true, I need to circle back to this in the AM. Seeing this on a personal project:
// lib.dom.d.ts
interface ShadowRootInit {
delegatesFocus?: boolean;
mode: ShadowRootMode;
}
splitinfinities
approved these changes
Oct 25, 2021
rwaskiewicz
changed the title
rwaskiewicz
added a commit
that referenced
this pull request
Jun 29, 2023
this commit fixes a bug where projects using the `dist-custom-elements`
output target with `externalRuntime: false` would receive the following
error when building their project:
```
[ WARN ] Bundling Warning SOURCEMAP_ERROR
Error when using sourcemap for reporting an error: Can't resolve original location of error.
```
the cause of this error was attempting to import a function,
`attachShadow` that is no longer exported from the runtime bundle (as of
#3117). to date, this has not
had an effect on stencil (as the import gets treeshaken away). however,
when trying to generate sourcemaps for a project using this
configuration would cause a mismatch between what was expected to be in
the produced output (the import statement) and what was really there (no
import statement)
2 tasks
rwaskiewicz
added a commit
that referenced
this pull request
Jul 13, 2023
this commit fixes a bug where projects using the `dist-custom-elements`
output target with `externalRuntime: false` would receive the following
error when building their project:
```
[ WARN ] Bundling Warning SOURCEMAP_ERROR
Error when using sourcemap for reporting an error: Can't resolve original location of error.
```
the cause of this error was attempting to import a function,
`attachShadow` that is no longer exported from the runtime bundle (as of
#3117). to date, this has not
had an effect on stencil (as the import gets treeshaken away). however,
when trying to generate sourcemaps for a project using this
configuration would cause a mismatch between what was expected to be in
the produced output (the import statement) and what was really there (no
import statement)
2 tasks
rwaskiewicz
added a commit
that referenced
this pull request
Jul 13, 2023
this commit fixes a bug where projects using the `dist-custom-elements`
output target with `externalRuntime: false` would receive the following
error when building their project:
```
[ WARN ] Bundling Warning SOURCEMAP_ERROR
Error when using sourcemap for reporting an error: Can't resolve original location of error.
```
the cause of this error was attempting to import a function,
`attachShadow` that is no longer exported from the runtime bundle (as of
#3117). to date, this has not
had an effect on stencil (as the import gets treeshaken away). however,
when trying to generate sourcemaps for a project using this
configuration would cause a mismatch between what was expected to be in
the produced output (the import statement) and what was really there (no
import statement)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 13, 2023
* fix(compiler): sourcemap generation for non-external runtime
this commit fixes a bug where projects using the `dist-custom-elements`
output target with `externalRuntime: false` would receive the following
error when building their project:
```
[ WARN ] Bundling Warning SOURCEMAP_ERROR
Error when using sourcemap for reporting an error: Can't resolve original location of error.
```
the cause of this error was attempting to import a function,
`attachShadow` that is no longer exported from the runtime bundle (as of
#3117). to date, this has not
had an effect on stencil (as the import gets treeshaken away). however,
when trying to generate sourcemaps for a project using this
configuration would cause a mismatch between what was expected to be in
the produced output (the import statement) and what was really there (no
import statement)
* refactor(compiler): remove unused ATTACH_SHADOW API
this commit removes the RUNTIME_APIS.ATTACH_SHADOW field that is no
longer used in the codebase. it's only usage was in tests for adding
runtime apis, and has been replaced with another field from
`RUNTIME_APIS`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm test) were run locally and passednpm run test.karma.prod) were run locally and passednpm run prettier) was run locally and passedPull request type
Please check the type of change your PR introduces:
What is the current behavior?
delegatesFocusis not honored/applied in thedist-custom-elementsnor in thedist-custom-elements-bundleoutput targets (it is simply ignored)GitHub Issue Number: #3002
What is the new behavior?
this commit allows
delegatesFocusto be properly applied to componentsgenerated using the following output targets:
the generation of the
attachShadowcall is moved from a standalonefunction to being attached to the prototype of the custom element when
we proxy it. the reason for this is that we need the component metadata
to to determine whether or not each individual component should have
delegateFocus enabled or not.
this change also led to the transition from using ts.create*() calls to
ts.factory.create*() calls for nativeAttachShadowStatement, which is the
general direction I'd like to take such calls, since the former is now
deprecated
Does this introduce a breaking change?
this led to the removal of the original standalone attachShadow
function. I do not consider this to be a breaking change, as we don't
publicly state our runtime APIs are available for general consumption.
Testing
I've added unit tests and karma tests for this change. The unit tests validate that we output a custom element component correctly (it calls
attachShadowin the constructor), while the karma tests ensure that we set thedelegatesFocusfield properly whenever we use the shadow DOM.I manually tested this by building and packing this branch (
npm ci && npm run build && npm pack) and installed it in https://github.com/Amiiiirali/delegates-focus-test (the repro for the linked GH Issue). Runningnpm ci && npm run build && npm startin Chrome, Edge, FF (Beta, see other information section below) and Safari confirms the fixOther information
delegatesFocusis implemented in FireFox 94. At the time of this writing, v94 is only available on the FireFox beta channel. Testing in FF requires a beta build in order to verify this all works.Docs update: ionic-team/stencil-site#781