-
Notifications
You must be signed in to change notification settings - Fork 792
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): re-arrange inheritance tree to fix type issue #5543
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
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
localName
different for Document and Element types. This fails because Document inherits from Element (wrongly IMHO) and so TypeScript complains because it overwriteslocalName
in Document to a property that returnsundefined
when the parent classElement
expects astring
return type.What is the new behavior?
I shuffled around the inheritance structure a bit to make this work. Document should inherit from
MockNode
as a super class and bothDocument
andElement
should 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