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

feat(web/lifecycleService): correct startupKind #206563

Merged
merged 6 commits into from
Mar 4, 2024

Conversation

samdenty
Copy link
Contributor

@samdenty samdenty commented Feb 29, 2024

This PR fixes #206345 & #206296.

Before the startupKind was only implemented for electron, but we can also do it on web (only found out cause chatgpt suggested). This makes the editor restoration logic better (and i'm sure other parts that use startupKind too)


Review PR in StackBlitz Submitted with StackBlitz.

@samdenty samdenty changed the title fix feat(web/lifecycleService): correct startupKind Feb 29, 2024
@bpasero bpasero added this to the Backlog milestone Mar 1, 2024
@samdenty samdenty requested a review from bpasero March 1, 2024 18:31
@bpasero bpasero modified the milestones: Backlog, March 2024 Mar 4, 2024
@bpasero
Copy link
Member

bpasero commented Mar 4, 2024

@samdenty any thoughts on https://web.dev/articles/navigation-and-resource-timing#acquiring_timings_in_application_code. I wonder about the actual perf impact of calling performance.getEntriesByType('navigation'). In my testing on macOS, I could not find this call to take long, but I wanted to point out the blog post that suggests a observer pattern.

@samdenty
Copy link
Contributor Author

samdenty commented Mar 4, 2024

iterating over a large number of entries, or even repeatedly polling the performance buffer to find new entries

I think since this we're calling performance.getEntriesByType('navigation') there should only ever be one event, so browsers shouldn't have a hard time resolving that. If it was a different type, then yeah that could be an issue.

The observer pattern seems to be for when you want to subscribe to the latest events, instead of polling the API

@bpasero
Copy link
Member

bpasero commented Mar 4, 2024

👍 , I think this can go in, I pushed some slight 👄 changes.

One thing that will be received as some kind of regression potentially is that in web, we no longer just restore the previous view you had opened (you removed the isWeb check). But on the other hand, that is consistent with the behaviour on desktop and as such actually the intended behaviour. I hope this will not backfire though...

@bpasero
Copy link
Member

bpasero commented Mar 4, 2024

Interesting: 5727c82

@bpasero bpasero merged commit bd60317 into microsoft:main Mar 4, 2024
6 checks passed
yiliang114 pushed a commit to yiliang114/vscode that referenced this pull request Mar 6, 2024
* fixes microsoft#206345

* fixes microsoft#206296

* fix: don't hard check isWeb

* chore: review

* cleanup

* cleanup

---------

Co-authored-by: Benjamin Pasero <[email protected]>
@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support LifecycleService startupKind in web
3 participants