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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Jun 5, 2024
Merged

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

merged 12 commits into from
Jun 5, 2024

Conversation

alicelovescake
Copy link
Member

@alicelovescake alicelovescake commented Mar 31, 2024 •
edited
Loading

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 in the last 24 hours 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 in the last 24 hours labels Apr 5, 2024
codebytere
codebytere reviewed Apr 16, 2024
View reviewed changes
Copy link
Member

@codebytere codebytere left a comment •
edited
Loading

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
Copy link
Member

I'd be in favor of adding deprecation warnings now (2) as to give developers and library authors the most time to migrate.

@alicelovescake
Copy link
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!

@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 reviewed May 27, 2024
View reviewed changes
Copy link
Member

@codebytere codebytere left a comment

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
Copy link
Member Author

alicelovescake commented May 28, 2024 •
edited
Loading

@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

codebytere
codebytere approved these changes May 29, 2024
View reviewed changes
Copy link
Member

@codebytere codebytere left a comment

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
Copy link
Member

@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

API LGTM

docs/api/web-contents.md Show resolved Hide resolved
docs/api/web-contents.md Show resolved Hide resolved
docs/api/web-contents.md Show resolved Hide resolved
docs/api/web-contents.md Outdated Show resolved Hide resolved
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
@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
Copy link
Member

@erickzhao erickzhao left a comment

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
Copy link

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

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
13 of 14 checks passed
@release-clerk Release Clerk
Copy link

release-clerk bot commented Jun 5, 2024

Release Notes Persisted

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@electron-docs-reviewer electron-docs-reviewer[bot] electron-docs-reviewer[bot] approved these changes

@erickzhao erickzhao erickzhao approved these changes

@codebytere codebytere codebytere approved these changes

@samuelmaddock samuelmaddock samuelmaddock approved these changes

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