Skip to content

fix: remove moment from force interop packages#11502

Merged
patak-cat merged 1 commit intovitejs:mainfrom
fi3ework:fix-10039
Jan 4, 2023
Merged

fix: remove moment from force interop packages#11502
patak-cat merged 1 commit intovitejs:mainfrom
fi3ework:fix-10039

Conversation

@fi3ework
Copy link
Member

@fi3ework fi3ework commented Dec 27, 2022 •
edited
Loading

Description

fix #10039.
The context could be found here #1724 (comment).
esbuild seems smarter now and won't wrap moment as a CommonJS module anymore. So the workaround could be dropped now.
I tested moment@2.29.1 which is released on 2020-10-06 and the latest version, and both works fine.

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

@ArnaudBarre
Copy link
Member

ArnaudBarre commented Dec 27, 2022

I think this is good to remove this edge case. Is this breaking for some older version of momentjs?

@fi3ework
Copy link
Member Author

fi3ework commented Dec 28, 2022

@ArnaudBarre

I tested moment@2.29.1 which is released on 2020-10-06 and the latest version, and both works fine.

Tested more versions:

  • 2.0.0, published 9 years ago, correct with a CJS wrapper.
  • 2.24.0, published 4 years ago, with high download, correct without a CJS interop.
  • 2.29.1, published 2 years ago, with high download, correct without a CJS interop.
  • 2.29.4, published 6 months ago, correct without a CJS interop.

As moment@1 should be very rare nowadays. I think it's acceptable to remove the hack now.

@bluwy bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label Dec 28, 2022
@bluwy bluwy added this to the 4.1 milestone Dec 31, 2022
@patak-cat patak-cat merged commit b89ddd6 into vitejs:main Jan 4, 2023
@fi3ework fi3ework deleted the fix-10039 branch January 4, 2023 16:03
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
@fi3ework @futurGH
fix: remove moment from force interop packages (vitejs#11502)
f8b1fdb
walmon referenced this pull request Mar 16, 2023
@yyx990803
fix(optimizer): handle special case where esm entry gets converted to…
32413ce 
… cjs by esbuild

fix #1724
@bluwy bluwy mentioned this pull request May 24, 2023
Merged
4 tasks
@ArnaudBarre ArnaudBarre mentioned this pull request Aug 5, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benmccann benmccann benmccann approved these changes

@bluwy bluwy bluwy approved these changes

@sapphi-red sapphi-red sapphi-red approved these changes

Assignees

No one assigned

Labels

p2-nice-to-have Not breaking anything but nice to have (priority)

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

Calling import * as a function in development should not work

6 participants

@fi3ework @ArnaudBarre @benmccann @bluwy @sapphi-red @patak-cat