Skip to content

feat(web/lifecycleService): correct startupKind#206563

Merged
bpasero merged 6 commits intomicrosoft:mainfrom
samdenty:unexpected-mouse
Mar 4, 2024
Merged

feat(web/lifecycleService): correct startupKind#206563
bpasero merged 6 commits intomicrosoft:mainfrom
samdenty:unexpected-mouse

Conversation

@samdenty
Copy link
Contributor

@samdenty samdenty commented Feb 29, 2024 •
edited
Loading

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
@samdenty samdenty mentioned this pull request Feb 29, 2024
Closed
@bpasero bpasero added this to the Backlog milestone Mar 1, 2024
bpasero
bpasero requested changes Mar 1, 2024
View reviewed changes
@samdenty samdenty requested a review from bpasero March 1, 2024 18:31
bpasero
bpasero requested changes Mar 4, 2024
View reviewed changes
@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 •
edited
Loading

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 •
edited
Loading

👍 , 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
yiliang114 pushed a commit to yiliang114/vscode that referenced this pull request Mar 6, 2024
@samdenty @bpasero @yiliang114
feat(web/lifecycleService): correct startupKind (microsoft#206563)
d23da11 
* fixes microsoft#206345

* fixes microsoft#206296

* fix: don't hard check isWeb

* chore: review

* cleanup

* cleanup

---------

Co-authored-by: Benjamin Pasero <benjamin.pasero@microsoft.com>
@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.

Reviewers

@bpasero bpasero bpasero approved these changes

@mjbvz mjbvz mjbvz approved these changes

Assignees

@bpasero bpasero

Labels

None yet

Projects

None yet

Milestone

March 2024

Development

Successfully merging this pull request may close these issues.

Support LifecycleService startupKind in web

3 participants

@samdenty @bpasero @mjbvz