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

refactor: update future options #18011

Merged
merged 2 commits into from
Sep 3, 2024
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Sep 3, 2024

Description

From #16471 (comment) (removed false because it's confusing)

This change is what I figured future options could be. We can discuss whether this is what we want or not here.

export interface FutureOptions {
  removePluginHookHandleHotUpdate?: 'warn'
  removePluginHookSsrArgument?: 'warn'

  removeServerModuleGraph?: 'warn'
  removeServerHot?: 'warn'
  removeServerTransformRequest?: 'warn'

  removeSsrLoadModule?: 'warn'
}

My idea is that:

  • future options contain a list of top-level properties (feature change names), and users have a choice to opt-in to that breaking change.
  • The signature will be future.<name>?: "warn" or future.<name>?: true
    • "warn" variant is used only when we're unsure this is a change we'll land. Users can try out by opting to "warn".
    • true variant is used when we're sure of this change, and you can opt into this breaking change now. Warnings are already emitted regardless of the option, but true would make them hard errors.

Reading the major changes documentation, I could probably see why it landed with future.deprecationWarnings in the first place, so this PR may make that page a little odd. My suggestion (if we go along with this) is that in the major changes page:

  • If a feature is "warn", put into the "Future" section (Rename as "Considering" to not overload the future term?)
  • If a feature is true, put into the "Current" section
  • If a feature is part of previous major, or we walk back on it, put into the "Past" section (Maybe want a "Rejected" section?)

Copy link

stackblitz bot commented Sep 3, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

I like the flat list, and also the new names for the sections in the docs. I think we can try this out, it feels more flexible in a way that we can add later on other kind of changes here (that maybe aren't about a deprecation but a change in the way some API works).

Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

Yeah, I like that. I see later we could potentially have error for some of them to throw error with stacktrace etc.

Do you think we should have an option to turn on all? Like this maybe:

future: {
  '*': 'warn'
}

@patak-dev
Copy link
Member

One thing we were discussing is that maybe we could have 'warn' | 'error' be the values instead of 'warn' | true. I like that they are both strings, but maybe true ends up being more correct later on, specially for behavior changes where we can not really error out and people will just see a different result. I think we can merge this for now and we can keep iterating until the release.

@patak-dev patak-dev merged commit 6443e48 into v6/environment-api Sep 3, 2024
8 checks passed
@patak-dev patak-dev deleted the update-future-options branch September 3, 2024 20:17
@bluwy
Copy link
Member Author

bluwy commented Sep 4, 2024

One thing we were discussing is that maybe we could have 'warn' | 'error' be the values instead of 'warn' | true

I'm fine with that too and we can iterate on it later.

Do you think we should have an option to turn on all? Like this maybe:

future: {
  '*': 'warn'
}

I'm fine supporting that too. I intentionally didn't include supporting it here since I wonder if it's often users want this. If we add a new option to future.* it'll almost always likely to break. I think it'll be more convenient for testing only than something someone commits to git.

@bluwy bluwy mentioned this pull request Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants