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

fix: support babel.config.js #72

Merged
merged 1 commit into from
Nov 26, 2018

Conversation

fobbyal
Copy link
Contributor

@fobbyal fobbyal commented Nov 23, 2018

What: Added logic to recognize babel.config.js as a source of babel config

Why: Latest babel documentation recommends babel.config.js (stating that babel itself uses this method). https://babeljs.io/docs/en/configuration

How: By adding additional !hasFile logic to the babel.js file

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is great! I wonder though if we should also just make kcd-scripts built-in config compile node_modules as well.

@fobbyal
Copy link
Contributor Author

fobbyal commented Nov 24, 2018

I am not very clear on what you mean.

  1. Compiled when using rollup only?
  2. how would i know which modules needs to be compiled? (via command-line parameters?)
  3. Do we need to hold up this PR for the node_modules feature?

@kentcdodds
Copy link
Owner

I think it actually should just take an update to the rollup config to remove the exclude reference to node_modules.

@fobbyal fobbyal force-pushed the support-babel.config.js branch from 6c01921 to 825444b Compare November 25, 2018 19:43
@fobbyal
Copy link
Contributor Author

fobbyal commented Nov 25, 2018

i amended my commit. I do have one question though. Does this mean the build will get a performance hit since babel will try to compile everything? (Sorry i am no babel expert)

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thanks! Yes, there will be a slight hit, but for the use cases of kcd-scripts that will amount to a few hundred milliseconds at most and only at build time so we're fine.

@@ -115,7 +115,8 @@ const output = [
},
]

const useBuiltinConfig = !hasFile('.babelrc') && !hasFile('.babelrc.js') && !hasPkgProp('babel')
Copy link
Owner

Choose a reason for hiding this comment

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

Could you update this to allow for the babel.config.js as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh sorry, I didn't catch that. I am not really a rollback user exactly. I am copying kcd-scripts to make something that feels more natural to me. Thanks for all the effort you put into the community!!!

1. added additional !hasFile logic to recognize babel.config.js in babel
script
2. added additional !hasFile logic to reconize babel.config.js in rollup
script
3. removed the exclude reference to node_modules in rollup script so
depdencies can also be compiled by babel
@fobbyal fobbyal force-pushed the support-babel.config.js branch from 825444b to 0a46371 Compare November 25, 2018 23:44
Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Super! Thank you!!

@kentcdodds kentcdodds merged commit ce74241 into kentcdodds:master Nov 26, 2018
@kentcdodds
Copy link
Owner

🎉 This PR is included in version 0.46.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants