-
Notifications
You must be signed in to change notification settings - Fork 795
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
fix(mock-doc): add HTMLUListElement #5169
Conversation
|
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 |
add a definition for `HTMLUListElement`. this allows `createElement` calls to return a mocked version of `HTMLUListElement` that passes `instanceof` calls. the `type` and `compact` properties were not implemented, as they are deprecated in the spec. prior to this commit, folks attempting to use an `HTMLUListElement` in a stencil test like so would receive an error: ```ts describe('my-component', () => { it('using HTMLUListElement works', async () => { const unorderedList = document.createElement('ul'); console.log(unorderedList instanceof HTMLUListElement); }); }); ``` ``` [46:08.8] jest args: --spec --e2e --max-workers=8 FAIL src/my-component.spec.ts my-component ✕ using HTMLUListElement works (4 ms) ● my-component › using HTMLUListElement works ReferenceError: HTMLUListElement is not defined 2 | it('using HTMLUListElement works', async () => { 3 | const unorderedList = document.createElement('ul'); > 4 | console.log(unorderedList instanceof HTMLUListElement); | ^ 5 | }); 6 | }); 7 | at Object.<anonymous> (src/my-component.spec.ts:4:42) ``` Fixes: #3382 STENCIL-466 bug: HTMLUListElement is not defined
116d416
to
4b8649d
Compare
|
||
describe('ul', () => { | ||
it('textContent', () => { | ||
const elm: MockUListElement = doc.createElement('ul'); |
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.
There aren't any unique properties here to test - so I just went with textContent
to test the creation of something
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.
👍
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.
LGTM
What is the current behavior?
When Stencil's mock-doc creates an
HTMLUListElement
, we create an element that does not descend fromHTMLUListElement
Fixes: #3382
What is the new behavior?
add a definition for
HTMLUListElement
. this allowscreateElement
calls to return a mocked version ofHTMLUListElement
that passesinstanceof
calls.prior to this commit, folks attempting to use an
HTMLUListElement
in a stencil test like so would receive an error:Does this introduce a breaking change?
Testing
I used the original reproduction case in the linked issue.
For posterity, it had a test that matched the one in the "New Behavior"/commit message:
If this test were placed in a new Stencil Component Starter project (
npm init stencil@latest component ul-test && cd $_ && npm i
, then copy the test to any*.spec.ts
file), then the tests run (npm t
), we'd see the failure above.After building this branch and packing it (
npm run clean && npm ci && npm run build && npm pack
) and installing the tarball into the component starter project (npm i <TARBALL>
), the tests pass (npm t
)Other information
STENCIL-466 bug: HTMLUListElement is not defined