feat: duplicate navigation related APIs to `contents.navigationHistory` by alicelovescake · Pull Request #41752 · electron/electron · GitHub
Skip to content

feat: duplicate navigation related APIs to contents.navigationHistory - #41752

Merged
VerteDinde merged 12 commits into
electron:mainfrom
alicelovescake:refactor-navigation-history
Jun 5, 2024
Merged

feat: duplicate navigation related APIs to contents.navigationHistory#41752
VerteDinde merged 12 commits into
electron:mainfrom
alicelovescake:refactor-navigation-history

Conversation

@alicelovescake

@alicelovescake alicelovescake commented Mar 31, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Description of Change

This PR refactors and duplicates navigation related APIs to contents.navigationHistory. Follow up to previous PR #41577 where a new navigationHistory property is created on web contents.

This PR duplicates and exposes the following API for consistency and clarity:

  • canGoBack
  • goBack
  • canGoForward
  • goForward
  • canGoToOffset
  • goToOffset
  • clear

Added related tests and documentation.

For backwards compatibility, the above APIs can still be directly accessed from webContents.

Checklist

Release Notes

Notes: Added the following existing navigation related APIs to webcontents.navigationHistory: canGoBack, goBack, canGoForward, goForward, canGoToOffset, goToOffset, clear

@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Mar 31, 2024
@alicelovescake alicelovescake changed the title refactor: move navigation related APIs to contents.navigationHistory refactor: duplicate navigation related APIs to contents.navigationHistory Mar 31, 2024
@alicelovescake alicelovescake changed the title refactor: duplicate navigation related APIs to contents.navigationHistory feat: duplicate navigation related APIs to contents.navigationHistory Mar 31, 2024
@dsanders11 dsanders11 added the semver/minor backwards-compatible functionality label Apr 5, 2024
@electron-cation electron-cation Bot added api-review/requested 🗳 and removed new-pr 🌱 PR opened recently labels Apr 5, 2024
codebytere
codebytere reviewed Apr 16, 2024
View reviewed changes

@codebytere codebytere left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Codewise this looks great to me - the only outstanding question here is what (if any) timeline we'd want to consider targeting for deprecation of the old methods. Our options I would say are:

  1. Merge this as is and support both for the medium-term future, deprecating the old approach in a handful of versions and removing a few after that
  2. Deprecate now to indicate that developers should look to use the new code where possible but leave in for a good long time, no specific removal date
  3. Deprecate and remove in the next few targeted cycles

My personal approach would probably be #2 but I think any are reasonably justifiable. I do think we should decided on one before merging though! Thoughts (cc @MarshallOfSound)?

@samuelmaddock

samuelmaddock commented May 2, 2024

Copy link
Copy Markdown
Member

@alicelovescake

alicelovescake commented May 3, 2024

Copy link
Copy Markdown
Member Author

Thanks for the feedback! @codebytere @samuelmaddock

I updated the PR with _Deprecated_ in the docs to let developers know to use the new api.

In the future, when we are ready to officially deprecate, I can:

  • update docs/breaking-changes.md
  • move the api to internal-electron.d.ts

Let me know if there is anything else I can do for deprecation warnings for this PR!

@alicelovescake
alicelovescake force-pushed the refactor-navigation-history branch from 5a21c21 to 8bfebec Compare May 5, 2024 00:50
@codebytere codebytere added target/30-x-y PR should also be added to the "30-x-y" branch. target/31-x-y PR should also be added to the "31-x-y" branch. labels May 13, 2024
@codebytere
codebytere requested a review from samuelmaddock May 27, 2024 12:58
codebytere
codebytere reviewed May 27, 2024
View reviewed changes

@codebytere codebytere left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@alicelovescake sorry for the delay here - in addition to adding deprecation notes to docs, we'll also want to leverage our internal deprecate module to notify developers at run-time as well:

deprecate.warnOnce('webContents.canGoToOffset', 'webContents.navigationHistory.canGoToOffset');

@alicelovescake
alicelovescake force-pushed the refactor-navigation-history branch from 3a3556a to 5e931eb Compare May 28, 2024 21:31
@alicelovescake

alicelovescake commented May 28, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

@alicelovescake sorry for the delay here - in addition to adding deprecation notes to docs, we'll also want to leverage our internal deprecate module to notify developers at run-time as well:

deprecate.warnOnce('webContents.canGoToOffset', 'webContents.navigationHistory.canGoToOffset');

Thanks for the code pointer! Added the warnings by:

  • Moved and renamed existing navigation APIs to internal. (i.e webContents.canGoBack => webContents._canGoBack)
  • Added javascript wrappers to emit the deprecation warning: I.e
const canGoBackDeprecated = deprecate.warnOnce('webContents.canGoBack', 'webContents.navigationHistory.canGoBack');
WebContents.prototype.canGoBack = function () {
  canGoBackDeprecated();
  return this._canGoBack();
};
  • Tested that warning works correctly: ⬇️
image

@alicelovescake
alicelovescake requested a review from codebytere May 28, 2024 21:36
codebytere
codebytere approved these changes May 29, 2024
View reviewed changes

@codebytere codebytere left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

API LGTM

samuelmaddock
samuelmaddock approved these changes May 29, 2024
View reviewed changes

@samuelmaddock samuelmaddock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

API LGTM

Comment thread docs/api/web-contents.md
Comment thread docs/api/web-contents.md
Comment thread docs/api/web-contents.md
Comment thread docs/api/web-contents.md Outdated
alicelovescake and others added 5 commits May 29, 2024 10:46
@alicelovescake @samuelmaddock
Update docs/api/web-contents.md
46743bd 
Co-authored-by: Sam Maddock <samuel.maddock@gmail.com>
@alicelovescake @samuelmaddock
Update docs/api/web-contents.md
62ffc88 
Co-authored-by: Sam Maddock <samuel.maddock@gmail.com>
@alicelovescake @samuelmaddock
Update docs/api/web-contents.md
7d2d01f 
Co-authored-by: Sam Maddock <samuel.maddock@gmail.com>
@alicelovescake @samuelmaddock
Update docs/api/web-contents.md
c7ba345 
Co-authored-by: Sam Maddock <samuel.maddock@gmail.com>
@alicelovescake
docs: fix links
56d56e8
erickzhao
erickzhao reviewed Jun 3, 2024
View reviewed changes
Comment thread docs/api/navigation-history.md Outdated
@alicelovescake
alicelovescake requested a review from a team as a code owner June 4, 2024 04:06
@codebytere
codebytere requested a review from erickzhao June 4, 2024 09:12
erickzhao
erickzhao approved these changes Jun 4, 2024
View reviewed changes

@erickzhao erickzhao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

docs: lgtm

electron-docs-reviewer[bot]
electron-docs-reviewer Bot approved these changes Jun 4, 2024
View reviewed changes

@electron-docs-reviewer electron-docs-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approving on behalf of the Electron Docs Team

@erickzhao erickzhao added no-backport and removed target/30-x-y PR should also be added to the "30-x-y" branch. target/31-x-y PR should also be added to the "31-x-y" branch. labels Jun 4, 2024
@VerteDinde
VerteDinde merged commit 406f644 into electron:main Jun 5, 2024
@release-clerk

release-clerk Bot commented Jun 5, 2024

Copy link
Copy Markdown

@ckerr ckerr mentioned this pull request Mar 31, 2026
Merged
5 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samuelmaddock samuelmaddock samuelmaddock approved these changes
@codebytere codebytere codebytere approved these changes
@electron-docs-reviewer electron-docs-reviewer[bot] electron-docs-reviewer[bot] approved these changes
+1 more reviewer
@erickzhao erickzhao erickzhao approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api-review/approved ✅ no-backport semver/minor backwards-compatible functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@alicelovescake @samuelmaddock @codebytere @erickzhao @VerteDinde @dsanders11