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

Allow file URLs to be used as import specifiers #9407

Merged
merged 2 commits into from
Dec 12, 2023
Merged

Allow file URLs to be used as import specifiers #9407

merged 2 commits into from
Dec 12, 2023

Conversation

matthewp
Copy link
Contributor

Changes

  • Wanted to support this pattern in integrations: addDevToolbarApp(new URL('./plugin.ts', import.meta.url))
  • This doesn't currently work because we directly pass this value into the virtual module.
  • Vite does not support file URLs.
  • A Vite plugin is the answer! Maybe we can talk them into adding this to core.

Testing

  • Test added

Docs

N/A, bug fix

Copy link

changeset-bot bot commented Dec 11, 2023

🦋 Changeset detected

Latest commit: 5fe9955

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Dec 11, 2023
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

I'm not sure how I feel about this... seems like integration hooks should handle conversions from whatever formats we want to support for entrypoints into a valid ID for Vite.

@matthewp
Copy link
Contributor Author

@natemoo-re I thought about doing it that way, but this is a lower-level solution and enables any vite plugin (in Astro) to use file URLs which is a super convenient way to support this sort of pattern (having some config value that gets piped into a virtual module). So I felt that was better.

We already allow full file paths as module specifiers, why not file URLs? It's the same thing, but more universal.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

I'm okay with supporting this! Still think we should fix the inconsistent integration entrypoint support, but that can be a follow-up

@matthewp
Copy link
Contributor Author

@natemoo-re can you explain more? Which integration APIs are inconsistent? I can maybe fix it up here. I just did a quick scan and it looks like all of them save what is passed in directly. Maybe they mutate it further down the stack?

Copy link
Contributor

@lilnasy lilnasy left a comment

Choose a reason for hiding this comment

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

Works well, simplifies things. Looks great to me!

@natemoo-re
Copy link
Member

@natemoo-re can you explain more? Which integration APIs are inconsistent? I can maybe fix it up here. I just did a quick scan and it looks like all of them save what is passed in directly. Maybe they mutate it further down the stack?

IMO that's the inconsistency? Some of them throw helpful errors if they can't resolve the file, some of them don't. Some of them support URL because they convert it to a path before storing it, some of them don't. Some support relative specifiers, some don't. My thought was to have a util that would normalize them or some way to ensure that they're all stored as a valid Vite ID.

@matthewp
Copy link
Contributor Author

@natemoo-re Do you know which ones behave differently? From looking at the code I don't see a difference.

@matthewp matthewp merged commit 546d92c into main Dec 12, 2023
13 checks passed
@matthewp matthewp deleted the file-url branch December 12, 2023 18:28
@astrobot-houston astrobot-houston mentioned this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants