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: duplicate navigation related APIs to contents.navigationHistory #41752

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alicelovescake
Copy link
Contributor

@alicelovescake alicelovescake commented Mar 31, 2024

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

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
Contributor 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review/requested 🗳 semver/minor backwards-compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants