Skip to content

preset-env: update modules default value#1820

Open
Hypnosphi wants to merge 1 commit into
babel:mainfrom
Hypnosphi:patch-1
Open

preset-env: update modules default value#1820
Hypnosphi wants to merge 1 commit into
babel:mainfrom
Hypnosphi:patch-1

Conversation

@Hypnosphi

Copy link
Copy Markdown

@babel-bot

Copy link
Copy Markdown
Contributor

Deploy preview for babel ready!

Built with commit 43265b2

https://deploy-preview-1820--babel.netlify.com

@existentialism

existentialism commented Sep 10, 2018

Copy link
Copy Markdown
Member

Thanks for the PR!

What are your thoughts on leaving the default value as-is, and instead, add a section atop it that describes how babel-loader@8 uses the caller option to set it to false automatically?

@Hypnosphi

Copy link
Copy Markdown
Author

It's just not true that default value is commonjs. modules: 'commonjs' and absense of modules option have different effects (the former won't be overridden by babel-loader)

@existentialism

existentialism commented Sep 11, 2018

Copy link
Copy Markdown
Member

🤷‍♂️ I'm open to what the rest of the team thinks. Ultimately, I agree that the introduction of the caller option makes the notion of "default" a little less clear.

I just think it's important to show both what the behavior is when using the preset directly and what can happen via the caller option... since it's not just babel-loader@8 that can use it.

@loganfsmyth

Copy link
Copy Markdown
Member

Maybe we can split the

defaults to `"commonjs"`.

into a new section that explains it explicitly?

@existentialism

Copy link
Copy Markdown
Member

Yeah, my initial thought was to just call out the fact that it can be different based on caller, and use babel-loader as an example, but I'm fine with replacing the defaults to bit completely.

@Hypnosphi

Hypnosphi commented Sep 11, 2018

Copy link
Copy Markdown
Author

“Allow edits from maintainers” is on, so please feel free to reword it

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.

4 participants