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

Refactor hydration path handling #4918

Merged
merged 15 commits into from
Oct 4, 2022
Merged

Refactor hydration path handling #4918

merged 15 commits into from
Oct 4, 2022

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Sep 29, 2022

Changes

Avoid usage of /@fs when compiling. We still have to embrace a leading slash for Windows as the compiler is resolving paths as URLs, but this will be patched after compiling and in vite-plugin-astro-postprocess.

Also undo .jsx stripping by compiler. We try to resolve to .tsx early during compiling now.

Ideally this patch won't be needed when I implement the next steps below.

Supersedes #4829

Next

I'll update @astrojs/compiler to:

  1. Expose resolvePath option from Astro compiler
  2. Remove URL path handling.
  3. Remove $$metadata.resolvePath (runtime astro/internal api), move all to compile-time.
  4. Remove .jsx stripping

(Appreciate thoughts on the plan too!)

Testing

All existing tests should pass.

Docs

N/A. internal refactor.

@changeset-bot
Copy link

changeset-bot bot commented Sep 29, 2022

🦋 Changeset detected

Latest commit: d3361a3

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 the pkg: astro Related to the core `astro` package (scope) label Sep 29, 2022
@AirBorne04 AirBorne04 mentioned this pull request Sep 30, 2022
1 task
@bluwy
Copy link
Member Author

bluwy commented Sep 30, 2022

So close to finishing this. Looks like Deno isn't happy with importing path :( I might have to find a workaround next week.

@bluwy
Copy link
Member Author

bluwy commented Oct 3, 2022

This is good to go now. I've updated the PR description for the details.

@bluwy bluwy marked this pull request as ready for review October 3, 2022 16:49
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.

Looks good! Happy to sync on removing some of these post-processing patches and handling most of this in the compiler.

Comment on lines +42 to +43
// TODO: Ideally the compiler could expose a `resolvePath` function so that we can
// unify how we handle path in a bundler-agnostic way.
Copy link
Member

Choose a reason for hiding this comment

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

Yep, this should be possible! Happy to sync on this as a follow-up.

@@ -76,6 +85,33 @@ export default function astro(_opts: AstroPluginOptions): Plugin {
);
return false;
},
visitObjectProperty: function (path) {
// Filter out none 'client:component-path' properties
Copy link
Member

Choose a reason for hiding this comment

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

Is this only needed because the compiler doesn't handle these properly yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. The compiler does handle the client:component-path but not in the way that this PR wants it, so we're postprocessing to have the correct path instead. We should be able to remove this as well when the compiler expose a resolvePath function so that we can resolve to the right one ourselves.

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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants