open-wc / custom-elements-manifest

Custom Elements Manifest is a file format that describes custom elements in your project.
https://custom-elements-manifest.open-wc.org/
243 stars 45 forks source link

Regression in `resolveInitializer` behavior in analyzer v0.9.1+ #242

Closed daneah closed 8 months ago

daneah commented 8 months ago

Checklist

Completing the items above will greatly improve triaging time of your issue.

Expected behavior I am seeing a regression to the behavior described in #83 as of v0.9.1+, which had to do with resolveInitializer. Pinning to 0.9.0 does not exhibit this issue. It seems the repro linked there is no longer available, and I'm not positive I'll be able to make a minimal one in a reasonable timeframe, but wanted to raise now in case it's readily clear why this might've cropped back up.

FWIW #83 was originally raised while working on pharos, which is where I'm running into it now—running yarn install && yarn build:core on the HEAD of main passes, while removing the package.json:resolutions object that's pinning to 0.9.0 and running yarn install && yarn build:core again results in the following error:

src/react-components/layout/pharos-layout.tsx:45:11 - error TS2304: Cannot find name 'PharosSpacingThreeAndAHalfX'.

45   rowGap: PharosSpacingThreeAndAHalfX,
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~

Found 1 error in src/react-components/layout/pharos-layout.tsx:45

(Where we expect rowGap: '3.5rem' instead)

thepassle commented 8 months ago

Would you be able to share the source code thats not being correctly analyzed?

daneah commented 8 months ago

Yes, sorry for not including that initially—the source code for that component, and the line that is seeing the changed behavior, live here.

That line references a variable from a built file, which will look like this:

...
export const PharosSpacingThreeAndAHalfX = "3.5rem";
...

Happy to provide further information as its useful!

thepassle commented 8 months ago

Are you sure this regression wasnt there in 0.9.0? If I revert back to the changes of 0.9.0 locally, I also see its not working:

image
daneah commented 8 months ago

I just tripled checked—starting from latest develop (which has resolutions set to 0.9.0 currently):

$ yarn why @custom-elements-manifest/analyzer
├─ @ithaka/pharos@workspace:packages/pharos [fbe57]
│  └─ @custom-elements-manifest/analyzer@npm:0.9.0 (via npm:0.9.0)
│
└─ @ithaka/pharos@workspace:packages/pharos
   └─ @custom-elements-manifest/analyzer@npm:0.9.0 (via npm:0.9.0)

$ yarn build:core # success

$ mv packages/pharos/custom-elements.json ~/custom-elements-0.9.0.json

Then, after updating the resolutions for the analyzer package to 0.9.1:

$ yarn install

$ yarn why @custom-elements-manifest/analyzer
├─ @ithaka/pharos@workspace:packages/pharos [fbe57]
│  └─ @custom-elements-manifest/analyzer@npm:0.9.1 (via npm:0.9.1)
│
└─ @ithaka/pharos@workspace:packages/pharos
   └─ @custom-elements-manifest/analyzer@npm:0.9.1 (via npm:0.9.1)

$ git diff
diff --git i/.yarn/install-state.gz w/.yarn/install-state.gz
index d4739cf..3b23e8c 100644
Binary files i/.yarn/install-state.gz and w/.yarn/install-state.gz differ
diff --git i/package.json w/package.json
index a248767..f2a80a2 100644
--- i/package.json
+++ w/package.json
@@ -120,7 +120,7 @@
     ]
   },
   "resolutions": {
-    "@custom-elements-manifest/analyzer": "0.9.0"
+    "@custom-elements-manifest/analyzer": "0.9.1"
   },
   "size-limit": [
     {
diff --git i/yarn.lock w/yarn.lock
index 3e0a332..4bbce79 100644
--- i/yarn.lock
+++ w/yarn.lock
@@ -2733,9 +2733,9 @@ __metadata:
   languageName: node
   linkType: hard

-"@custom-elements-manifest/analyzer@npm:0.9.0":
-  version: 0.9.0
-  resolution: "@custom-elements-manifest/analyzer@npm:0.9.0"
+"@custom-elements-manifest/analyzer@npm:0.9.1":
+  version: 0.9.1
+  resolution: "@custom-elements-manifest/analyzer@npm:0.9.1"
   dependencies:
     "@custom-elements-manifest/find-dependencies": "npm:^0.0.5"
     "@github/catalyst": "npm:^1.6.0"
@@ -2750,7 +2750,7 @@ __metadata:
   bin:
     cem: cem.js
     custom-elements-manifest: cem.js
-  checksum: 10c0/70fe7f39ff710a6abccda9008fa8d1220b92c515fe53e3f61ac2747960bc244e273758f297c4644d1a0397e0208c7e01796a55f6e1912c0473c8836b1c4584fd
+  checksum: 10c0/3d1d5decfbd9241d4e10d2a28be20e95599ec1e6ba52aa65ef0fd2af5830204b603407aba48658536be50297636990eaae1e8fbaafcc05f6cb3b356ded6dc1ac
   languageName: node
   linkType: hard

Confirming this is I think the only moving piece changed, then trying the build again:

$ yarn build:core
...
src/react-components/layout/pharos-layout.tsx:45:11 - error TS2304: Cannot find name 'PharosSpacingThreeAndAHalfX'.

45   rowGap: PharosSpacingThreeAndAHalfX,
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~

Found 1 error in src/react-components/layout/pharos-layout.tsx:45
$ mv packages/pharos/custom-elements.json ~/custom-elements-0.9.1.json

Looking at custom-elements-0.9.0.json, I see that it contains a resolveInitializer object:

"resolveInitializer": {
  "module": "/src/styles/variables"
},

Whereas custom-elements-0.9.1.json has no resolveInitializer blocks at all.

It's entirely possible how we've been extracting the values from custom-elements.json up to now, which relied on the presence of resolveInitializer, was brittle, because I do see this block is available in both versions and is where we get the value from when resolveInitializer is present:

{
  "kind": "variable",
  "name": "PharosSpacingThreeAndAHalfX",
  "type": {
    "text": "string"
  },
  "default": "\"3.5rem\""
},

So ultimately this might be an "us" issue to update our expectations of the data shape, and if you agree I'm happy to close this out!

thepassle commented 8 months ago

Ah yes, it looks like youre relying on internals that were accidentally leaked (and why they were removed). The resolveInitializers arent actually specced in the Custom Elements Manifest, but we use them to resolve initializers to get richer information. I could revert the change for now, im working on some stuff that will better handle this kind of thing in the future, but do be aware that youre relying on a non-standard field

daneah commented 8 months ago

Very good to know, thank you for the clarification! I would happily try to fix this on our end rather than imposing more work on you, especially if no one else has mentioned this yet, and now that I know a bit more detail.

thepassle commented 8 months ago

Sounds good 🙂 ill close this issue for now then. Thanks for the detailed information btw!