fix(mock-doc): re-arrange inheritance tree to fix type issue #5543
Conversation
|
| Path | Error Count |
|---|---|
| src/dev-server/index.ts | 37 |
| src/dev-server/server-process.ts | 32 |
| 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 | 17 |
| src/dev-server/request-handler.ts | 15 |
| src/compiler/prerender/prerender-optimize.ts | 14 |
| src/compiler/sys/stencil-sys.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 |
| src/compiler/output-targets/output-www.ts | 12 |
| src/compiler/transformers/test/parse-vdom.spec.ts | 12 |
| src/compiler/transformers/transform-utils.ts | 12 |
| src/compiler/transpile/transpile-module.ts | 12 |
| src/mock-doc/test/attribute.spec.ts | 12 |
Our most common errors
| Typescript Error Code | Count |
|---|---|
| TS2322 | 362 |
| TS2345 | 343 |
| TS18048 | 204 |
| TS18047 | 82 |
| TS2722 | 37 |
| TS2532 | 24 |
| TS2531 | 21 |
| TS2454 | 14 |
| TS2790 | 11 |
| TS2352 | 10 |
| TS2769 | 8 |
| TS2538 | 8 |
| TS2416 | 7 |
| TS2493 | 3 |
| 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 |
PR built and packed!Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8363578680/artifacts/1343581445 If your browser saves files to |
There was a problem hiding this comment.
While I agree this inheritance model as currently written doesn't seem to align well with the standard, I don't think we can change a class hierarchy like this without it being a breaking change - we end up removing all methods that are provided by MockHTMLElement & MockNode out of instances of MockDocument here.
I propose we take one of two different approaches here -
Option 1 - Throw for localName
We can change MockDocument's implementation of localName to throw instead of return undefined. Since localName doesn't/shouldn't actually exist on real Document instances, we can appease the type checker by "returning a never top-type" here:
diff --git a/src/mock-doc/document.ts b/src/mock-doc/document.ts
index e97c1bd3b..a5ada8570 100644
--- a/src/mock-doc/document.ts
+++ b/src/mock-doc/document.ts
@@ -49,8 +49,8 @@ export class MockDocument extends MockHTMLElement {
this.documentElement.dir = value;
}
- override get localName(): undefined {
- return undefined;
+ override get localName(): never {
+ throw new Error("Unimplemented");
}
get location() {
Option 2 - Widen localName return type
This one's a little more disingenuous, if not a little deceiving. In it, we can widen the MockElement return type to string | undefined. This would equally appease the type checker, and aligns with calling a getter that doesn't exist on an object:
diff --git a/src/mock-doc/node.ts b/src/mock-doc/node.ts
index 6c775831f..2f000d0ad 100644
--- a/src/mock-doc/node.ts
+++ b/src/mock-doc/node.ts
@@ -266,12 +266,22 @@ Testing components with ElementInternals is fully supported in e2e tests.`,
);
}
- get localName() {
+
+ // TODO(STENCIL-ABC): Refactor the inheritance tree
+ /**
+ * Implementation of the {@link https://developer.mozilla.org/en-US/docs/Web/API/Element/localName localName property}.
+ *
+ * Due to the way MockDoc is constructed, we widen the return type of this function to include `undefined`.
+ * This is not standards compliant, and will be fixed in a major version release of Stencil.
+ *
+ * @returns the node name, represented as a string
+ * @throws a new Error is thrown if an instance's `nodeName` property is not set.
+ */
+ get localName(): string | undefined {
/**
* The `localName` of an element should be always given, however the way
* MockDoc is constructed, it won't allow us to guarantee that. Let's throw
* and error we get into the situation where we don't have a `nodeName` set.
- *
*/
if (!this.nodeName) {
throw new Error(`Can't compute elements localName without nodeName`);The patches for each can be found below:
- Option 1 - widen-localname.patch
- Option 2 - throw-localname-doc.patch
I tested both options by building Stencil locally with each of the patches below, building the tarball, and installing it into an instance of Ionic Framework Core locally. I haven't run it through the entire Nightly pipeline, which might reveal one (or both) aren't actually tenable. LMK if you have any reservations or simply know these won't work 😅
@rwaskiewicz, could you please provide further explanation for your statement? In my opinion, I believe we are not removing any features that we advertise to provide. Our documentation does not explicitly mention that Stencil utilizes a JSDOM-like implementation of the DOM API. This absence may lead users to assume that the environment in which the unit tests run adheres strictly to HTML specifications. In fact by keeping the methods attached to the wrong type of objects we might cause false positives, so having this change (while possibly breaking) results in a better state after all. In addition to that, I don't see that much changing anyway, e.g.:
That said, I am all for carefully evaluating if changes we introduce can cause issues for users updating to latest version of Stencil. In this case however we are changing the interface towards a state that we advertise in our docs anyway. I would argue that fixing certain slot behavior are breaking changes too but we still move forward with them, without introducing a flag for every fix and even as patches, as we orient ourselves to the behavior defined by the Shadow DOM specification. I think this case is no different. I would prefer to move forward with this. The suggested alternatives punt on the issue at hand and even introduce more divergence between what we actually suggest to provide to user. That said, I am also not dying on a hill for this 😉 so you call! |
|
Completely understandable! If I'm honest, our documentation hasn't always been great, and we've put more burden on our users to figure things out than we should have in the past. While we've been working to fix that for some time now, we can't guarantee that folks haven't been relying on the existing incorrect behavior for some time. I agree continuing to do this may lead to/potentially compound some of confusion, but I think we should do this in a way that's potentially less invasive for folks. I can't prove that people are relying on this behavior, which is the tricky part here. It potentially makes all our arguments surrounding what is/is not a breaking change a judgement call. Here, I'd like to err on the side of caution until we've designed reordering these classes a bit more. As is, the current state of this branch removes support for a few things like We can see this with the following patch on a Stencil Component Starter output: diff --git a/src/components/my-component/my-component.tsx b/src/components/my-component/my-component.tsx
index 56d51d9..24d66d5 100644
--- a/src/components/my-component/my-component.tsx
+++ b/src/components/my-component/my-component.tsx
@@ -1,5 +1,6 @@
import { Component, Prop, h } from '@stencil/core';
import { format } from '../../utils/utils';
+import { MockDocument } from '@stencil/core/mock-doc';
@Component({
tag: 'my-component',
@@ -23,6 +24,12 @@ export class MyComponent {
@Prop() last: string;
private getText(): string {
+ const doc = new MockDocument();
+ // hidden is no longer available
+ doc.hidden
+ // neither is attachInternals, although this would be a very
+ // wrong thing for someone to do
+ doc.attachInternals()
return format(this.first, this.middle, this.last);
}I'm very open to pulling this into v5 - for the purposes of unblocking the nightly build, I think a simpler solution here is the better way to go |
|
@rwaskiewicz sounds good to me, let's label this PR so we don't forget to merge it in v5. In the meantime we can move forward with #5595. A dev release was cut that we can use to test against framework: |
rwaskiewicz
added
Stencil v5
What is the current behavior?
We see a breaking nightly build in https://github.com/ionic-team/ionic-framework/actions/runs/8338316083/job/22818424122 which happened due to the fact that I initially had to set
localNamedifferent for Document and Element types. This fails because Document inherits from Element (wrongly IMHO) and so TypeScript complains because it overwriteslocalNamein Document to a property that returnsundefinedwhen the parent classElementexpects astringreturn type.What is the new behavior?
I shuffled around the inheritance structure a bit to make this work. Document should inherit from
MockNodeas a super class and bothDocumentandElementshould inherit from there.Documentation
n/a
Does this introduce a breaking change?
I expect no breaking changes technically but can't guarantee.
Testing
I will make a dev build an re-run this in framework to double check.
Other information