Skip to content

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

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
Merged
VerteDinde merged 12 commits into from
Jun 5, 2024
Merged

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

VerteDinde 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 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
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

samuelmaddock commented May 2, 2024

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

alicelovescake commented May 3, 2024

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
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 alicelovescake force-pushed the refactor-navigation-history branch from 3a3556a to 5e931eb Compare May 28, 2024 21:31
@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

@alicelovescake alicelovescake requested a review from codebytere May 28, 2024 21:36
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

erickzhao
erickzhao reviewed Jun 3, 2024
View reviewed changes
@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
@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

@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