-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat(runtime): add preview.pathname
#233
Conversation
Run & review this pull request in StackBlitz Codeflow. |
ca63d5d
to
e7b4acf
Compare
e7b4acf
to
9ab8645
Compare
9ab8645
to
adefbd5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the config change!
I would prefer if we had _availablePreviews
set to Map<number, PortInfo>
instead which it should have been from the beginning. The idea behind _availablePreviews
was to track information about the active port
s from a webcontainer perspective.
However, when I had written the code I thought that most use case would probably have a single preview per port which is an over-simplification.
The correct approach is to use a different class in _availablePreviews
other than PreviewInfo
and make sure all PreviewInfo
instances point to that instance.
So that means:
class PortInfo {
constructor(
public readonly port: number,
public origin: string;
public ready: boolean
) {}
}
Then we modify PreviewInfo
to something like:
export class PreviewInfo {
- port: number;
- ready: boolean;
- baseUrl?: string;
+ readonly portInfo: PortInfo; // initialized in constructor
+
+ get port() { return this.portInfo.port; }
+ get baseUrl() { return this.portInfo.origin; }
Finally when setPreviews
is called we check if a PortInfo
is present in the Map
. If it isn't then we create one, and in webcontainer.on('port')
we update that instance instead.
We could also consider have the ready
and origin
fields in PortInfo
be atom
s which I think could make sense, however given that in the current code we signal changes on previews by setting the previews
atom
on PreviewsStore
to a new array instance, we don't really need those to be atom
I think.
I think we need to either make it If it was public field that allows overwriting, we would do it here: const previewInfos = previews.map((preview) => {
const info = new PreviewInfo(preview);
const portInfo = this._availablePreviews.get(info.port);
if (!portInfo) {
this._availablePreviews.set(info.port, info.portInfo);
} else {
info.portInfo = portInfo; // <-- All previews sharing same port should point to same portInfo
}
return info;
}); Or if we would track multiple |
I don't think I'm following that reasoning. A single instance of In your code snippet:
The logic should be changed to: const previewInfos = previews.map((preview) => {
const port = PreviewInfo.parsePort(preview);
let portInfo = this._availablePreviews.get(port);
if (!portInfo) {
portInfo = new PortInfo(port);
this._availablePreviews.set(port, portInfo);
}
return new PreviewInfo(preview, portInfo);
}); |
This looks good, I've applied these changes! This allows us to have multiple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good! 🤩
Really good job on this! 🎉
previews
definition #192Adds support for defining pathname for previews. Example from Vite Plugin Tutorial where I'd like to show two previews for same server:
This will also be useful for remult tutorial: https://learn.remult.dev/1-basics/5-live-query/2-realtime-updates - mentioned here #226