Skip to content

feat: rename plugin enforce to order#9670

Closed
antfu wants to merge 3 commits intomainfrom
feat/enfore-to-order
Closed

feat: rename plugin enforce to order#9670
antfu wants to merge 3 commits intomainfrom
feat/enfore-to-order

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-cat patak-cat added the p2-nice-to-have Not breaking anything but nice to have (priority) label Aug 14, 2022
@patak-cat patak-cat added this to the 3.1 milestone Aug 14, 2022
@patak-cat
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-cat
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-cat patak-cat modified the milestones: 3.1, 3.2 Sep 2, 2022
@patak-cat patak-cat 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