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: rename plugin enforce to order #9670

Closed
wants to merge 3 commits into from
Closed

Conversation

antfu
Copy link
Member

@antfu antfu commented Aug 14, 2022

Description

To align with Rollup's naming on object hooks. #9634

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Aug 14, 2022
@patak-dev patak-dev added this to the 3.1 milestone Aug 14, 2022
@patak-dev
Copy link
Member

I think we should also merge this one in Vite 3.1. We could start issuing a warning in Vite 4 when enforce is used, and then remove it next year in Vite 5. It would be nice to do it in v4, but it is a significant breaking change for a small maintenance cost.

Co-authored-by: patak <matias.capeletto@gmail.com>
@sapphi-red
Copy link
Member

As we now have hook level order, do we still need plugin level order/enforce?
IIUC plugin level order/enforce is just a shorthand of setting every hook's order. Is it common that the hook's order to be the same? (Now it is because we didn't have hook level order and it was the only way)
I feel aligning to rollup (removing the plugin level enforce completely) is better for the ecosystem.

I agree that we should deprecate enforce first and remove enforce in Vite 5.

@antfu
Copy link
Member Author

antfu commented Aug 24, 2022

I think the order of the plugin still matters.

  • A nitch case I'd say a post plugin's post hook is not equivalent to a pre plugin's post hooks.
  • It would be verbose if you want every hook to have the order.

It would be great if Rollup would like to introduce top-level order, but even if not, I still think it would be valuable for Vite given our application is more complex.

@sapphi-red
Copy link
Member

Ah, so there is 3 * 3 = 9 orders now. Am I correct?

  1. plugin level pre + hook level pre
  2. plugin level normal + hook level pre
  3. plugin level post + hook level pre
  4. plugin level pre + hook level null
  5. plugin level normal + hook level null
  6. plugin level post + hook level null
  7. plugin level pre + hook level post
  8. plugin level normal + hook level post
  9. plugin level post + hook level post

@patak-dev
Copy link
Member

Ah, so there is 3 * 3 = 9 orders now. Am I correct?

Yes, and the order you listed would be respected for Vite.

It would be verbose if you want every hook to have the order.

I think we should keep order at the plugin level because of this reason.

A nitch case I'd say a post plugin's post hook is not equivalent to a pre plugin's post hooks.

This feels a bit too niche to me. Still on the fence about what is better:

  1. order is only a plugin-level default for all hooks
  2. order both move the plugin and is a default for the hooks

I feel 1. is more consistent. If we want to attack that niche case, maybe is it better to rely on normal ordering, or to add more stages in the future? ('start', 'pre', 'normal', 'post', 'end'). The reason I like 2 is that I'm very used to see the plugin when we log things jump to the enforced position, but I expect this to change once we get used to 'order' at the hook level not doing this.

@patak-dev patak-dev modified the milestones: 3.1, 3.2 Sep 2, 2022
@patak-dev patak-dev modified the milestones: 3.2, 4.0 Sep 30, 2022
@antfu
Copy link
Member Author

antfu commented Oct 7, 2022

After the team meeting, we think it's better to hold this PR. Having it named enforce makes it clear it's a Vite-specific behavior and not confusing with Rollup's order. Also, avoiding collision in case Rollup might introduce the top-level order in the future.

@benmccann
Copy link
Collaborator

Having it named enforce makes it clear it's a Vite-specific behavior and not confusing with Rollup's order. Also, avoiding collision in case Rollup might introduce the top-level order in the future.

Does that mean this PR should be closed?

There's a merge conflict that would need to be resolved if this is still going to be pursued

@antfu antfu closed this Nov 8, 2022
@antfu
Copy link
Member Author

antfu commented Nov 8, 2022

Yes, I think we shall close for now.

@antfu antfu deleted the feat/enfore-to-order branch March 8, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants