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

use named parameters for node visit context info #3619

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented May 31, 2022

Motivation:

  1. Some visit context information is rarely used, this could be pathway toward refactoring, future-proofing any breaking changes to come. (see: Refactor visit function #3225)
  2. Eventually, BREAK and/or other visit controllers could be added as third argument directly to the visitor method rarely than exported from the library. (see Separate interfaces from implementations for exported classes #3616)

@netlify
Copy link

netlify bot commented May 31, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit d1e3180
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62990e7848527600095adb7f
😎 Deploy Preview https://deploy-preview-3619--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@yaacovCR
Copy link
Contributor Author

yaacovCR commented May 31, 2022

Motivation added above.

@yaacovCR yaacovCR added the PR: breaking change 💥 implementation requires increase of "major" version number label May 31, 2022
@yaacovCR yaacovCR requested a review from a team May 31, 2022 03:30
Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

@yaacovCR I'm totally for this change 👍
But it makes a breaking change without migration path and means you can't support v16 and v17 in the same code base.
Are we sure we want to do this?

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jun 6, 2022

Thanks for taking a look. We could try to rework as new format that is optional in v17 but required in v18.

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jun 6, 2022

Alternatively we could backport the backwards compatible version to v16 and just deprecate in v17

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jun 7, 2022

Backwards compatible version is easier said than done. We can't just change the second argument to keyOrContext, because the directionality is actually opposite, our users are supplying the function, rather than us writing it. How would we know which type of function we were supplied and whether to pass the key or the context?

What seems possible, but not necessarily the best choice: we could make our users specify in some way which type of visitor function they are passing, and then require them to pass it that way. Easiest but ugliest, we could create an entire new function called visitWithNamedVisitorFnArguments that supercedes visit and then deprecate visit.

Adding to next js-wg. If you are reading this, please feel free to chime in.

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jun 8, 2022

@yaacovCR As a suggestion to allow a faster review cycle, can we have a rule where PRs either blocked on the technical solution or WG discussion is marked as draft?
https://github.blog/changelog/2020-04-08-convert-pull-request-to-draft/

This will make the "Pull requests" tab more representative.

@yaacovCR yaacovCR marked this pull request as draft June 8, 2022 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: breaking change 💥 implementation requires increase of "major" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants