Skip to content

[v2, dev] Use custom schemes for in-app dev mode #2610

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

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

stffabi
Copy link
Collaborator

@stffabi stffabi commented Apr 18, 2023

This fixes some long-standing inconsistencies between dev mode builds and production builds but is a breaking change. Dev mode uses custom scheme for Vite versions >= 3.0.0 and for older it still behaves in the old way.

Relates to

@stffabi stffabi requested a review from leaanthony April 18, 2023 07:51
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Apr 18, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 182bf09
Status: ✅  Deploy successful!
Preview URL: https://2f489165.wails.pages.dev
Branch Preview URL: https://feature-dev-mode-custom-sche.wails.pages.dev

View logs

@leaanthony
Copy link
Member

This is great! Perhaps we should link the tickets we believe will be fixed by this?

@leaanthony
Copy link
Member

Do you think using the old URL for Vite < 3 would be reasonable? It'd mean it wouldn't break compatibility but we could spam the terminal with all the benefits of upgrading to v3?

@stffabi
Copy link
Collaborator Author

stffabi commented Apr 18, 2023

This is great! Perhaps we should link the tickets we believe will be fixed by this?

Yeah that's a good idea.

@stffabi
Copy link
Collaborator Author

stffabi commented Apr 18, 2023

Do you think using the old URL for Vite < 3 would be reasonable? It'd mean it wouldn't break compatibility but we could spam the terminal with all the benefits of upgrading to v3?

This would need some more if-cases so theoretically it's feasible. I'm not sure if that would give a better experience for the developer. In the current version of the PR it's clear that when upgrading to the next release there's a breaking change and the user needs to update vite. Otherwise it would still work as in older releases and would then introduce the breaking change just by the means of updating vite to a newer version.
As an end user of Wails I would personally prefer to have specific version that introduces the breaking change, and not by just updating Vite.
Another solution would to add a config value, but I don't know if that's really better. Especially we are only fixing an inconsistency in between dev/prod mode and all people that didn't encounter such an inconsistency by using a feature in dev that doesn't exist in prod should not be affected by this breaking change.

@stffabi
Copy link
Collaborator Author

stffabi commented Apr 18, 2023

I've update the initial comment to list issues that I've found relating to such inconsistencies.

@stffabi stffabi marked this pull request as ready for review April 18, 2023 11:07
@stffabi
Copy link
Collaborator Author

stffabi commented Apr 18, 2023

@leaanthony I've marked the PR ready for review, let me know how you would like to proceed with it.

@leaanthony
Copy link
Member

I think if it's not a huge problem, we should use the default behaviour if vite < 3 but issue a large warning about it. I think this will be a small minority because v3 has been standard in v2 for a while, however if their projects just fail to work then that would be a bad experience. There may be some weird reason they can't upgrade or perhaps they are using a remote template. I'm happy to do that work if you're done with it 😅

@stffabi
Copy link
Collaborator Author

stffabi commented Apr 18, 2023

Ok, will do that. Unfortunately we still had some old vite v2 in our templates. 😞

@stffabi stffabi force-pushed the feature/dev-mode-custom-schemes branch from 7987c1b to 25e0755 Compare April 18, 2023 12:34
@stffabi
Copy link
Collaborator Author

stffabi commented Apr 18, 2023

@leaanthony PR has been updated

@leaanthony
Copy link
Member

I remember from the first commit (can't get to it after force push) that ViteServerURLChan was used. According to my IDE, it looks like it's only sent a value but the value isn't read. Am I reading this wrong?

@stffabi
Copy link
Collaborator Author

stffabi commented Apr 19, 2023

I remember from the first commit (can't get to it after force push) that ViteServerURLChan was used. According to my IDE, it looks like it's only sent a value but the value isn't read. Am I reading this wrong?

Didn't change anything there, it's still being read here v2/cmd/wails/internal/dev/dev.go line 235

@leaanthony
Copy link
Member

That is so weird! I wonder if it was a gui issue with a small window as I can see it just fine on desktop... 🤯

Copy link
Member

@leaanthony leaanthony left a comment

Choose a reason for hiding this comment

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

Thanks so much for adding in the legacy support 🙏 This feature should help a lot.

This fixes some long-standing inconsistencies between
dev mode builds and production builds but is a breaking
change. Dev mode uses custom scheme for Vite versions >= 3.0.0
and for older it still behaves in the old way.
@stffabi stffabi force-pushed the feature/dev-mode-custom-schemes branch from 25e0755 to 182bf09 Compare April 20, 2023 10:07
@stffabi
Copy link
Collaborator Author

stffabi commented Apr 20, 2023

You're welcome. I hope this will get us rid of all those "working in development, not working in production" tickets.

@stffabi stffabi merged commit 529ec56 into master Apr 20, 2023
@stffabi stffabi deleted the feature/dev-mode-custom-schemes branch April 20, 2023 10:37
leaanthony pushed a commit that referenced this pull request Apr 26, 2023
This fixes some long-standing inconsistencies between
dev mode builds and production builds but is a breaking
change. Dev mode uses custom scheme for Vite versions >= 3.0.0
and for older it still behaves in the old way.
@leaanthony leaanthony mentioned this pull request Nov 18, 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