Skip to content
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

vite also needs to resolve index.{gjs,gts} #1826

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

patricklx
Copy link
Contributor

@patricklx patricklx commented Feb 29, 2024

blocked by #1876

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will likely be fixed by #1794, or at the very least we should add a failing test somewhere after that is merged before we make any more changes like this 👍

@@ -1,2 +1,8 @@
<template>Yay for gts!</template>
import WithIndex from './with-index';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the import that would fail to resolve

@patricklx patricklx force-pushed the patch-9 branch 4 times, most recently from 195b2cb to 954cd44 Compare February 29, 2024 13:21
@patricklx
Copy link
Contributor Author

one test failed while fetching node...

@ef4
Copy link
Contributor

ef4 commented Mar 5, 2024

Idea here is that we should try switching at least the gjs/gts support to rely on vote's own resolve-extensions, so that it composes with vite's built-in index search.

We may or may not be able to do that for hbs given that within app code the hbs support is more complex, given the existence of template-only components, etc.

@patricklx
Copy link
Contributor Author

somehow after adding the extensions to resolve.
the module-resolver resolveComponent is receiving type: ignore results...

@patricklx
Copy link
Contributor Author

patricklx commented Mar 27, 2024

the commit 5559000 makes esbuild resolver behave the same as vite, when we have a not-found we report it as external.
now another issue:
all imports that are resolved via resolveWithinMovedPackage do not get dep optimization, because the importer is in node_modules. this causes the vite app to use duplicate modules @ef4

@patricklx
Copy link
Contributor Author

patricklx commented Mar 27, 2024

I think for virtual files we want to add a suffix instead of a prefix. It should then be able to generate correct imports that use the dep optimization

@patricklx
Copy link
Contributor Author

patricklx commented Apr 2, 2024

2 issues:

  • resolveWithinMovedPackage changes importer to be in node-modules, which makes vite/esbuild ignore it.
  1. solution: generate links to rewritten-packages that are resolvable via node resolution and just rewrite the request specifier
  2. Change the specifier to a special prefix that will be treated specially? How?
  • virtual modules start with a prefix, which changes all its imports to @id/fs/... full path. Can be fixed by using a suffix instead.

@ef4
Copy link
Contributor

ef4 commented Apr 2, 2024

I think this is going to need pairing at office hours to sort through. Changing from prefix to suffix seems like a workaround for what is actually breaking.

@patricklx
Copy link
Contributor Author

Screenshot 2024-04-04 at 16 00 51

@patricklx patricklx marked this pull request as draft April 6, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants