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.

Sign up for GitHub

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

Jump to bottom

fix(commonjs): add .cjs to default file extensions. #524

Merged
merged 6 commits into from Aug 6, 2020
Merged

fix(commonjs): add .cjs to default file extensions. #524

merged 6 commits into from Aug 6, 2020

Conversation

ctjlewis
Copy link
Contributor

@ctjlewis ctjlewis commented Aug 2, 2020 •
edited

Rollup Plugin Name: commonjs

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers: #523

Description

Adds .cjs to default file extensions.

@ctjlewis ctjlewis changed the title Added .cjs and .mjs to default file extensions. refactor(commonjs): added .cjs and .mjs to default file extensions. Aug 2, 2020
@ctjlewis ctjlewis mentioned this pull request Aug 2, 2020
Closed
shellscape
shellscape requested changes Aug 3, 2020
View reviewed changes
Copy link
Collaborator

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

Thanks for opening a PR. Before we can prosede, we'll need you to add relevant tests. That's a hard requirement for all fix and feature PRs. This also isn't a refactor - you're attempting to fix an issue by adding code.

packages/commonjs/yarn.lock Outdated Show resolved Hide resolved
@shellscape shellscape changed the title refactor(commonjs): added .cjs and .mjs to default file extensions. fix(commonjs): add .cjs and .mjs to default file extensions. Aug 3, 2020
@lukastaegert
Copy link
Member

While I very much agree that we need to add .cjs, what was your reasoning of adding .mjs as well? After all by their definition, they cannot contain CommonJS elements.

@ctjlewis
Copy link
Contributor Author

ctjlewis commented Aug 3, 2020 •
edited

@lukastaegert You're right, that definitely doesn't make sense - will update. Very sorry about the issues here, TY for your review as well @shellscape.

@ctjlewis ctjlewis changed the title fix(commonjs): add .cjs and .mjs to default file extensions. fix(commonjs): add .cjs to default file extensions. Aug 3, 2020
shellscape
shellscape previously approved these changes Aug 3, 2020
View reviewed changes
@shellscape shellscape self-requested a review August 4, 2020 14:40
@shellscape shellscape dismissed their stale review August 4, 2020 14:41

recent comments

@ctjlewis ctjlewis requested a review from Andarist August 4, 2020 23:16
lukastaegert
lukastaegert approved these changes Aug 6, 2020
View reviewed changes
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Looks good from my side

@shellscape shellscape merged commit d7f06e9 into rollup:master Aug 6, 2020
@ctjlewis ctjlewis deleted the cjs-mjs-extensions branch August 6, 2020 15:45
LarsDenBakker pushed a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020
@LarsDenBakker
fix(commonjs): add .cjs to default file extensions. (rollup#524)
148342b 
* Added .cjs and .mjs to default file extensions.

* Removed accidental yarn.lock.

* Added test.

* Added snapshots.

* Removed unneeded extension.

* Moved extension check, refactored Array.indexOf() to Array.includes()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukastaegert lukastaegert lukastaegert approved these changes

@LarsDenBakker LarsDenBakker Awaiting requested review from LarsDenBakker

@NotWoods NotWoods Awaiting requested review from NotWoods

@btd btd Awaiting requested review from btd

@pnevares pnevares Awaiting requested review from pnevares

@TrySound TrySound Awaiting requested review from TrySound

@wuweiweiwu wuweiweiwu Awaiting requested review from wuweiweiwu

@shellscape shellscape Awaiting requested review from shellscape shellscape is a code owner

@Andarist Andarist Awaiting requested review from Andarist

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ctjlewis @lukastaegert @shellscape @Andarist