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

Hot Reload Vue on shared/ changes #240

Merged
merged 8 commits into from
Apr 21, 2024
Merged

Conversation

benjaminpjones
Copy link
Collaborator

@benjaminpjones benjaminpjones commented Apr 19, 2024

Trying to figure out #28 (hot reload when shared/ changes)

While this was the one way I found to get hot reload to work, I don't think this solution is great for a couple of reasons.

  • "@/../../shared" - it's pretty hacky to use relative imports. It means we'll need to change a ton of files if we change the directory structure.
    • I'd prefer to import from "@shared" or something
    • I tried fiddling with "paths" and other similar config options, but no luck
  • Would be better if we could depend on dist instead of src

That said, I am well beyond the limit of my TS and Vue/Vite knowledge, I am hoping someone might see a better path now that we have a "working" path

Hot Reload Demo

Notice I am editing a variable in shared/src/index.ts!!!

Screen.Recording.2024-04-19.at.6.09.36.PM.mp4

@JonKo314
Copy link
Collaborator

JonKo314 commented Apr 19, 2024

  • it's pretty hacky to use relative imports

Yeah

  • Would be better if we could depend on dist instead of src

I'm not so sure about that one. src has all the typed stuff and doesn't need to be compiled, but it's just a feeling, I don't know enough about it.

It's very cool that you got it to work, but it would be sad if there wasn't a nicer way that just works.

@benjaminpjones
Copy link
Collaborator Author

benjaminpjones commented Apr 19, 2024

Would be better if we could depend on dist instead of src

I'm not so sure about that one. src has all the typed stuff and doesn't need to be compiled, but it's just a feeling, I don't know enough about it.

So I don't feel too strongly about this point compared to the first one. However, my thought process is that dist is the directory that consumers would normally use when importing from the package ("@ogfcommunity/variants-shared"). It also means the typescript settings in shared/ need to match vue-client. There are a couple settings that are mismatched like "verbatimModuleSyntax".

it would be sad if there wasn't a nicer way that just works.

Indeed! I'll need some help though - I've tried quite a few ways with very little luck 😞

@benjaminpjones
Copy link
Collaborator Author

benjaminpjones commented Apr 19, 2024

Okay actually, I think I got the absolute imports figured out. Using ./shared instead of shared in compilerOptions.paths helps 😃

Not sure why "@shared/index" works, but "@shared" doesn't, but much better than "@/../../shared" imo. Edit: likely a moduleResolution thing

@benjaminpjones
Copy link
Collaborator Author

benjaminpjones commented Apr 20, 2024

Okay, I think I actually got this in a place that's publishable.

Summary of changes since earlier conversation:

  • figured out how to keep the imports the same as before (not relative)
  • I changed back to @ogfcommunity/variants-shared from @shared
    • not because I'm married to that name, but I wanted to keep focus on config changes in this PR
  • Reverted tsconfig.vitest.json because it's of no consequence

I didn't come up with a solution to depend on the dist folder, but as @JonKo314 pointed out, there are good reasons to depend on src anyway. One side-effect is that I needed to annotate type-only imports in shared/src/index.ts because Vue seems to need it.

@benjaminpjones benjaminpjones marked this pull request as ready for review April 20, 2024 03:54
@benjaminpjones benjaminpjones changed the title [RFC] Import @/../../shared instead of @ogfcommunity/shared Hot Reload Vue on shared/ changes Apr 20, 2024
</script>

<template>
<main>
<h2>Welcome to Go variants!</h2>
<h2>Welcome to {{ SITE_NAME }}!</h2>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was mainly for testing. I can remove it before merging, but want to make sure we landed on an agreeable configuration first.

}
,
"verbatimModuleSyntax": false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think verbatimModuleSyntax off makes it so I don't need to put type { SomeImport } everywhere in shared/. I'm not opposed to it, but it would be quite a large change, and I wanted to keep this diff to config-only as much as possible.

@@ -4,9 +4,14 @@
"exclude": ["src/**/__tests__/*"],
"compilerOptions": {
"composite": true,
"baseUrl": ".",
"baseUrl": "..",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This puts us in the packages directory, so we can look at packages/shared

@@ -3,6 +3,14 @@
"include": ["vite.config.*", "vitest.config.*", "cypress.config.*"],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm pretty sure the changes were required in all 3 tsconfig files (excluding tsconfig.vitest.json). Maybe there is a way to dedupe using the "extends" field. But I don't quite understand the difference in intent behind app and config anyway... Maybe we can remove them and just keep tsconfig.json?

@@ -9,6 +9,12 @@ export default defineConfig({
resolve: {
alias: {
"@": fileURLToPath(new URL("./src", import.meta.url)),
"@ogfcommunity/variants-shared/src": fileURLToPath(
new URL("../shared/src", import.meta.url),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ogfcommunity/variants-shared/src is to support imports like "@ogfcommunity/variants-shared/src/time_control"

@ogfcommunity/variants-shared/src is to support imports like "@ogfcommunity/variants-shared".

Importantly, the /src version needs to come first, else the latter will overtake it.

@benjaminpjones benjaminpjones merged commit 20776d1 into main Apr 21, 2024
3 checks passed
@benjaminpjones benjaminpjones deleted the attempt-fix-new-path branch April 23, 2024 05:14
@benjaminpjones benjaminpjones mentioned this pull request Apr 28, 2024
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.

2 participants