vite test ci #1718

patricklx · 2023-12-12T22:01:25Z

adds ci tests for vite dev

testing: opening /tests/ and checking the tests output
fixes optimizer issue

packages/compat/src/compat-app-builder.ts

mansona · 2023-12-13T13:06:19Z

@patricklx I don't see this running in CI 🤔 are you just working on this to be able to run tests locally?

patricklx · 2023-12-13T13:08:19Z

the target is to be able to run it in ci. this is just the setup for it.
No idea where to setup the ci test here in embroider repo...

patricklx · 2023-12-13T13:14:43Z

@mansona also tests will not work without

import * as EmberTesting from 'ember-testing';
define('ember-testing', () => EmberTesting);

unless we use ember-source 5.6 beta

mansona · 2023-12-13T13:20:38Z

No idea where to setup the ci test here in embroider repo...

So everything happens in scenario-tester which you can see in tests/scenarios/*-test.ts. for now I think we could start up a new scenario that would run this

patricklx · 2023-12-13T13:23:44Z

i might need to extract more from my other PR to make this work

patricklx · 2023-12-13T13:30:43Z

If you could setup the scenario, then i can fix the rest :)

packages/vite/src/resolver.ts

patricklx · 2023-12-14T15:38:29Z

packages/core/src/module-resolver.ts

@@ -542,12 +542,22 @@ export class Resolver {
  }

  private *componentJSCandidates(inPackageName: string) {
+    const extensions = ['.ts', '.gjs', '.gts'];


vite can handle this files. webpack can also already handle ts files, for gjs/gts a loader could be added

patricklx · 2023-12-14T15:39:17Z

packages/vite/src/template-tag.ts

-      let resolution = await this.resolve(id, importer, {
-        skipSelf: true,
-      });
+      // prevent resolve loop during vite build


this parts are only needed during vite build. otherwise it will hang

tests/scenarios/vite-app-test.ts

patricklx · 2023-12-15T15:11:42Z

the tests pass locally when i run them directly in the repo in windows. But through the scenario setup it fails locally as well. Anyway dep optimizer is not working correctly.
Disabled windows for now. Will look into it later again.
@mansona @ef4

patricklx · 2023-12-17T16:10:21Z

@mansona found a fix for windows.

patricklx · 2023-12-18T23:19:10Z

@mansona this is ready and also shows that its working while also building cached deps

ef4

Adding the CI test for vite is great. But that can be one PR by itself. The tests in vite-app are already passing on main, so no other changes should be needed to enable the tests in CI.

To make these other changes to core features like PackageCache or module-resolver we need tests showing why they're needed, and to ensure we don't break those cases in the future.

packages/shared-internals/src/package-cache.ts

packages/vite/src/hbs.ts

packages/vite/src/resolver.ts

packages/vite/src/template-tag.ts

packages/vite/package.json

patricklx · 2023-12-19T19:13:47Z

Adding the CI test for vite is great. But that can be one PR by itself. The tests in vite-app are already passing on main, so no other changes should be needed to enable the tests in CI.

To make these other changes to core features like PackageCache or module-resolver we need tests showing why they're needed, and to ensure we don't break those cases in the future.

The tests where not really testing much. Only that it loads... There where issues with decorators and vite build was also not working.
But yes, i can minimise this to only do vite test first on unix only, and extract the rest to other prs like this:
current pr: only enable ci tests
rest:

create more tests
test windows
add gjs/gts entr for tests
build (if i cannot find a simple solution)

patricklx · 2023-12-21T10:44:12Z

packages/vite/src/esbuild-resolver.ts

@@ -30,6 +30,9 @@ export function esBuildResolver(root = process.cwd()): EsBuildPlugin {
        let resolution = await resolverLoader.resolver.resolve(request, defaultResolve(build, kind));
        switch (resolution.type) {
          case 'found':
+            if (resolution.result?.path?.includes('rewritten-app')) {


@mansona I i think i found the real reason we have the optimizer issue.
We call context.resolve in the default resolve here 0aafe02#diff-a6c8071691a61acc68be864b34d3f338fd92e9bf5217c2d12403ac6029bca885R130
with the full absolute path.
which then goes to the vite dep:scan plugin which marks it as external here: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/optimizer/scan.ts#L681

so either we do not call that and check if the id is already resolved in the default resolve
or use e.g resolve from npm or unmark them as external here.

i also created a PR on vite as i think its a vite bug: vitejs/vite#15402
lets see

Btw, this was working in windows without issues... Will verify later again why this is. I suspect the generated path has somr difference in slashes

patricklx · 2023-12-21T12:53:45Z

@mansona @ef4 this is ready

ef4 · 2023-12-21T15:24:53Z

packages/core/src/module-resolver.ts

      let resolution = this.nodeResolve(
        `${target.packageName}${candidate.prefix}${target.memberName}${candidate.suffix}`,
        target.from
      );
      if (resolution.type === 'real') {
-        hbsModule = resolution.filename;
+        hbsModule = candidateSpecifier;


This is similar to what we did in #1648. Both are cases where we were rewriting requests to absolute paths, which ends up looking to vite like the user actually typed an absolute path in their import, which gets different behavior than a normal relative- or package-name import.

In this case when we successfully resolve a specifier to a component file to check that it exists, rather than rewriting the request to point directly at that file we rewrite the request to use the specifier that we just successfully resolved.

(This duplicate resolving is only needed when supporting the ambiguity of loose-mode templates. As people migrate to template tag they stop needing it.)

patricklx commented Dec 12, 2023

View reviewed changes

packages/compat/src/compat-app-builder.ts Outdated Show resolved Hide resolved

patricklx mentioned this pull request Dec 13, 2023

[vite] use transform instead of load for gjs & hbs files #1680

Merged

patricklx force-pushed the vite-test branch 4 times, most recently from ca4d35d to 616ddf8 Compare December 14, 2023 15:37

patricklx commented Dec 14, 2023

View reviewed changes

patricklx force-pushed the vite-test branch from 616ddf8 to 5747458 Compare December 14, 2023 15:40

mansona reviewed Dec 14, 2023

View reviewed changes

tests/scenarios/vite-app-test.ts Outdated Show resolved Hide resolved

patricklx force-pushed the vite-test branch from f3bc46e to 4f5056d Compare December 15, 2023 09:57

patricklx mentioned this pull request Dec 15, 2023

[vite entry point] add gjs + gts extension for resolvable extensions #1679

Closed

patricklx force-pushed the vite-test branch 2 times, most recently from b00cd4f to abd8bbb Compare December 15, 2023 13:28

patricklx force-pushed the vite-test branch 9 times, most recently from cdb9a41 to fef309d Compare December 18, 2023 22:10

patricklx force-pushed the vite-test branch 3 times, most recently from 046eaeb to 13bc884 Compare December 18, 2023 23:18

patricklx force-pushed the vite-test branch 2 times, most recently from d06ba7b to 212eb9a Compare December 19, 2023 14:01

ef4 requested changes Dec 19, 2023

View reviewed changes

patricklx closed this Dec 19, 2023

patricklx reopened this Dec 19, 2023

patricklx force-pushed the vite-test branch from 15e800e to 0aafe02 Compare December 21, 2023 10:40

patricklx commented Dec 21, 2023

View reviewed changes

patricklx force-pushed the vite-test branch 3 times, most recently from de8a7d6 to 94977ab Compare December 21, 2023 12:18

patricklx added 2 commits December 21, 2023 13:24

add ci tests for vite

3cf4bf1

fix optimizer

7e3ac0d

patricklx force-pushed the vite-test branch from 94977ab to 7e3ac0d Compare December 21, 2023 12:24

patricklx and others added 2 commits December 21, 2023 15:15

add explaining comment

b8043bf

don't alias to absolute paths

01e7862

ef4 reviewed Dec 21, 2023

View reviewed changes

ef4 approved these changes Dec 21, 2023

View reviewed changes

virtual case should still use full paths

070928b

ef4 merged commit 2edb355 into embroider-build:main Dec 21, 2023
203 checks passed

NullVoxPopuli added enhancement New feature or request internal labels Dec 21, 2023

github-actions bot mentioned this pull request Dec 21, 2023

Prepare Release #1701

Closed

github-actions bot mentioned this pull request Apr 4, 2024

Prepare Release #1865

Closed

github-actions bot mentioned this pull request Apr 15, 2024

Prepare Release #1873

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vite test ci #1718

vite test ci #1718

patricklx commented Dec 12, 2023 •

edited

mansona commented Dec 13, 2023

patricklx commented Dec 13, 2023

patricklx commented Dec 13, 2023

mansona commented Dec 13, 2023

patricklx commented Dec 13, 2023

patricklx commented Dec 13, 2023

patricklx Dec 14, 2023

patricklx Dec 14, 2023

patricklx commented Dec 15, 2023 •

edited

patricklx commented Dec 17, 2023

patricklx commented Dec 18, 2023

ef4 left a comment

patricklx commented Dec 19, 2023 •

edited

patricklx Dec 21, 2023 •

edited

patricklx Dec 21, 2023

patricklx Dec 21, 2023

patricklx commented Dec 21, 2023

ef4 Dec 21, 2023

vite test ci #1718

vite test ci #1718

Conversation

patricklx commented Dec 12, 2023 • edited

mansona commented Dec 13, 2023

patricklx commented Dec 13, 2023

patricklx commented Dec 13, 2023

mansona commented Dec 13, 2023

patricklx commented Dec 13, 2023

patricklx commented Dec 13, 2023

patricklx Dec 14, 2023

Choose a reason for hiding this comment

patricklx Dec 14, 2023

Choose a reason for hiding this comment

patricklx commented Dec 15, 2023 • edited

patricklx commented Dec 17, 2023

patricklx commented Dec 18, 2023

ef4 left a comment

Choose a reason for hiding this comment

patricklx commented Dec 19, 2023 • edited

patricklx Dec 21, 2023 • edited

Choose a reason for hiding this comment

patricklx Dec 21, 2023

Choose a reason for hiding this comment

patricklx Dec 21, 2023

Choose a reason for hiding this comment

patricklx commented Dec 21, 2023

ef4 Dec 21, 2023

Choose a reason for hiding this comment

patricklx commented Dec 12, 2023 •

edited

patricklx commented Dec 15, 2023 •

edited

patricklx commented Dec 19, 2023 •

edited

patricklx Dec 21, 2023 •

edited