Add v8 coverage & upgrade to Vite 5 #300

M4tiz · 2024-06-17T09:55:20Z

No description provided.

wouterlucas

👍

M4tiz · 2024-06-18T14:10:07Z

@wouterlucas any ideas why the test pipeline fails? It doesn't look like my PR influences it.
https://github.com/lightning-js/renderer/actions/runs/9545663743/job/26329904984?pr=300

frank-weindel · 2024-06-18T15:09:00Z

Investigating this. This change is somehow producing errors during the TypeScript build of the renderer when built within the VRT runner --ci container:

> @lightningjs/[email protected] build /work
> tsc --build

node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/index.d.ts:843:22 - error TS2420: Class 'import("/work/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/index").FSWatcher' incorrectly implements interface 'import("fs").FSWatcher'.
  Type 'FSWatcher' is missing the following properties from type 'FSWatcher': ref, unref

843 export declare class FSWatcher extends EventEmitter implements fs.FSWatcher {
                         ~~~~~~~~~

src/core/text-rendering/font-face-types/TrFontFace.ts:185:7 - error TS2353: Object literal may only specify known properties, and 'variant' does not exist in type 'FontFaceDescriptors'.

185       variant: descriptors.variant,
          ~~~~~~~

src/core/text-rendering/font-face-types/WebTrFontFace.ts:60:7 - error TS2353: Object literal may only specify known properties, and 'variant' does not exist in type 'FontFaceDescriptors'.

60       variant: determinedDescriptors.variant,
         ~~~~~~~

node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/index.d.ts:843:22 - error TS2420: Class 'import("/work/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/index").FSWatcher' incorrectly implements interface 'import("fs").FSWatcher'.
  Type 'FSWatcher' is missing the following properties from type 'FSWatcher': ref, unref

843 export declare class FSWatcher extends EventEmitter implements fs.FSWatcher {
                         ~~~~~~~~~

src/core/text-rendering/font-face-types/TrFontFace.ts:185:7 - error TS2353: Object literal may only specify known properties, and 'variant' does not exist in type 'FontFaceDescriptors'.

185       variant: descriptors.variant,
          ~~~~~~~

src/core/text-rendering/font-face-types/WebTrFontFace.ts:60:7 - error TS2353: Object literal may only specify known properties, and 'variant' does not exist in type 'FontFaceDescriptors'.

60       variant: determinedDescriptors.variant,
         ~~~~~~~


Found 6 errors.

frank-weindel · 2024-06-18T15:47:36Z

Okay. This issue is reproducible locally if you delete all the node_modules folders and run pnpm install from the repo root.

What seems to be happening is that the new dependency is pulling in [email protected] (likely somehow via peerDependency), whereas prior to this change [email protected] would get installed.

Vite 4.5.2 seemed to have introduced this bug which seems to be fixed in Vite 5, but never backported to Vite 4:

type error | Type 'FSWatcher' is missing the following properties from type 'FSWatcher': ref, unref vitejs/vite#15714 (comment)

Upgrading to Vite 5 may solve this issue. If you could help try that, that would be greatly appreciated and we could get this PR merged sooner. Thanks 🙏

frank-weindel · 2024-06-18T15:58:32Z

Actually not sure if that will solve the variant issue. I don't have time to look at this further atm.

wouterlucas · 2024-06-21T13:07:47Z

Nice! I'd rebase this against dev - thats the working branch we're on and will be part of the next big "release".

That way we don't even have to bother with stuff like:

[dev] 3:06:17 PM [vite] warning:
[dev] ./workspace/renderer/dist/src/render-drivers/utils.js
[dev] 13 |      try {
[dev] 14 |          console.log('Loading core extension', coreExtensionModule);
[dev] 15 |          module = (await import(coreExtensionModule /* @vite-ignore */));
[dev]    |                                 ^^^^^^^^^^^^^^^^^^^
[dev] 16 |      }
[dev] 17 |      catch (e) {
[dev] The above dynamic import cannot be analyzed by Vite.
[dev] See https://github.com/rollup/plugins/tree/master/packages/dynamic-import-vars#limitations for supported dynamic import formats. If this is intended to be left as-is, you can use the /* @vite-ignore */ comment inside the import() call to suppress this warning.

😄

M4tiz · 2024-06-21T13:29:19Z

@frank-weindel @wouterlucas
I decided to try to update to Vite 5.x I hope that's alright :)

Crucial changes:

Removed variant field since the new FontFaceDescriptors contract doesn't have it anymore. It seems to be compliant with https://www.w3.org/TR/css-fonts-3/#font-face-rule specification
Changed tsconfig module resolution to Node16 in all variants due to Cannot find module 'rollup/parseAst' or its corresponding type declarations. rollup/rollup#5199

Let me know any of these require some additional checks to look for regression.

frank-weindel · 2024-06-21T14:16:10Z

src/core/utils.ts

@@ -204,9 +204,9 @@ export const getTimingFunction = (
  }

  if (timingLookup[str] !== undefined) {
+    const [a, b, c, d] = timingLookup[str];


Is the @ts-ignore for this no longer needed? If so can you delete the comments related to it?

It is still needed but for the line 211:
const timing = getTimingBezier(a, b, c, d);

frank-weindel · 2024-06-21T14:20:17Z

@frank-weindel @wouterlucas I decided to try to update to Vite 5.x I hope that's alright :)

Crucial changes:

Removed variant field since the new FontFaceDescriptors contract doesn't have it anymore. It seems to be compliant with https://www.w3.org/TR/css-fonts-3/#font-face-rule specification

Changed tsconfig module resolution to Node16 in all variants due to Cannot find module 'rollup/parseAst' or its corresponding type declarations. rollup/rollup#5199

Let me know any of these require some additional checks to look for regression.

@M4tiz Excellent. Thanks! That's much appreciated.

Looks like there's an error in CI with this change related to the examples pnpm lock file:

Scope: all 3 workspace projects
 ERR_PNPM_OUTDATED_LOCKFILE  Cannot install with "frozen-lockfile" because pnpm-lock.yaml is not up to date with examples/package.json

Note that in CI environments this setting is true by default. If you still need to run install in such cases, use "pnpm install --no-frozen-lockfile"

    Failure reason:
    specifiers in the lockfile ({"@lightningjs/renderer":"link:..","@stdlib/random-base-mt1[9](https://github.com/lightning-js/renderer/actions/runs/9614491706/job/26521477915?pr=300#step:7:10)937":"^0.2.1","@lightningjs/vite-plugin-import-chunk-url":"^0.3.0","vite":"^5.0.0"}) don't match specs in package.json ({"vite":"^5.0.0","@lightningjs/renderer":"link:.."})

frank-weindel · 2024-06-25T18:44:05Z

pnpm-lock.yaml

      playwright:
        specifier: ^1.39.0
-        version: 1.39.0
+        version: 1.44.1


Something seems a bit off about how you're pnpm installing/upgrading the dependencies needed for this PR. I just did it myself on a test branch and I'm not seeing any version changes to any unrelated packages like this one in the pnpm-lock.yaml file. Are you using a mix of npm/pnpm by accident by any chance? The package-lock.json file should not be being updated.

It seems like the reason you're getting the visual regression failures is that your changes are updating playwright to this newer version which seems to change how things are rendering. I'd advise that you start the new dependency installation from scratch and avoid the unnecessary changes.

You're right. It must've entered this state while I was tinkering with the dependencies. It's fixed now :)

Thanks. Looks good!

wouterlucas requested a review from frank-weindel June 17, 2024 11:00

wouterlucas approved these changes Jun 18, 2024

View reviewed changes

M4tiz changed the title ~~Add v8 coverage~~ Add v8 coverage & upgrade to Vite 5 Jun 21, 2024

M4tiz changed the base branch from main to dev June 21, 2024 13:14

M4tiz changed the base branch from dev to main June 21, 2024 13:14

M4tiz force-pushed the test/coverage branch from f200fd2 to 53f88c8 Compare June 21, 2024 13:21

M4tiz changed the base branch from main to dev June 21, 2024 13:21

M4tiz force-pushed the test/coverage branch from 53f88c8 to 3c14b3e Compare June 21, 2024 13:32

frank-weindel reviewed Jun 21, 2024

View reviewed changes

M4tiz force-pushed the test/coverage branch from 3c14b3e to ac9c7c4 Compare June 21, 2024 14:49

frank-weindel reviewed Jun 25, 2024

View reviewed changes

M4tiz force-pushed the test/coverage branch from de0d890 to 1f4ab9c Compare June 26, 2024 09:46

Add v8 coverage

cb4bef9

M4tiz force-pushed the test/coverage branch from 1f4ab9c to 875219a Compare June 26, 2024 09:49

Upgrade to Vite 5

a0805bf

M4tiz force-pushed the test/coverage branch from 875219a to a0805bf Compare June 26, 2024 14:18

frank-weindel merged commit a26008d into lightning-js:dev Jun 26, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add v8 coverage & upgrade to Vite 5 #300

Add v8 coverage & upgrade to Vite 5 #300

M4tiz commented Jun 17, 2024

wouterlucas left a comment

M4tiz commented Jun 18, 2024

frank-weindel commented Jun 18, 2024 •

edited

Loading

frank-weindel commented Jun 18, 2024 •

edited

Loading

frank-weindel commented Jun 18, 2024

wouterlucas commented Jun 21, 2024 •

edited

Loading

M4tiz commented Jun 21, 2024

frank-weindel Jun 21, 2024

M4tiz Jun 21, 2024

frank-weindel commented Jun 21, 2024

frank-weindel Jun 25, 2024

M4tiz Jun 26, 2024

frank-weindel Jun 26, 2024

Add v8 coverage & upgrade to Vite 5 #300

Add v8 coverage & upgrade to Vite 5 #300

Conversation

M4tiz commented Jun 17, 2024

wouterlucas left a comment

Choose a reason for hiding this comment

M4tiz commented Jun 18, 2024

frank-weindel commented Jun 18, 2024 • edited Loading

frank-weindel commented Jun 18, 2024 • edited Loading

frank-weindel commented Jun 18, 2024

wouterlucas commented Jun 21, 2024 • edited Loading

M4tiz commented Jun 21, 2024

frank-weindel Jun 21, 2024

Choose a reason for hiding this comment

M4tiz Jun 21, 2024

Choose a reason for hiding this comment

frank-weindel commented Jun 21, 2024

frank-weindel Jun 25, 2024

Choose a reason for hiding this comment

M4tiz Jun 26, 2024

Choose a reason for hiding this comment

frank-weindel Jun 26, 2024

Choose a reason for hiding this comment

frank-weindel commented Jun 18, 2024 •

edited

Loading

frank-weindel commented Jun 18, 2024 •

edited

Loading

wouterlucas commented Jun 21, 2024 •

edited

Loading