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

Use custom Babel loader to avoid using separate Babel copies for loader and loader options #4417

Merged
merged 4 commits into from
May 23, 2018

Conversation

loganfsmyth
Copy link
Contributor

@loganfsmyth loganfsmyth commented May 17, 2018

This resolves the

.value is not a valid Plugin property

error showing up for people in #4227

cc @timneutkens

@rgbkrk
Copy link
Contributor

rgbkrk commented May 22, 2018

Thank you for helping pave the path for next.js' babel setup @loganfsmyth! Is there a defined pattern you'd want next.js users to do when we have some additional changes to how babel and webpack are used?

@loganfsmyth
Copy link
Contributor Author

@rgbkrk I think it depends on what your goal is for the additional changes.

If you want to be able to add additional plugins and such, I'd personally say it would be up to Next to expose a config.babel handler could either

  • Be called for every file, and passed into the loader implemented in this PR
  • Be called once up front to provide extra config values to merge into Next's config.

However, since your changes seem more focused on your specific usecase of compiling a specific set of node_modules, what you have isn't too bad. My main concern however is that your logic there relies on the using having configured a .babelrc, since your logic on its own will compile those modules, but not enable any plugins/presets on its own.

@rgbkrk
Copy link
Contributor

rgbkrk commented May 22, 2018

I'd personally say it would be up to Next to expose a config.babel handler could either

That seems like a really smart idea so that it remains consistent like this PR.

However, since your changes seem more focused on your specific usecase of compiling a specific set of node_modules, what you have isn't too bad. My main concern however is that your logic there relies on the using having configured a .babelrc, since your logic on its own will compile those modules, but not enable any plugins/presets on its own.

I'd love to ask you more questions in the nteract monorepo if it's alright so I don't end up derailing your PR within next.js. As long as you've got the cycles, I'd love to figure out an ideal setup since I'm also transitioning us to babel 7 right now. Really happy to outline what we're doing and ensure we help others with monorepo + next + custom babel. It's ok if not, I'm sure you're plenty busy with lots of projects.

@loganfsmyth
Copy link
Contributor Author

@rgbkrk No problem, happy to try to answer any questions you have over there. Babel's Slack is also public if chat would be easier for quick back-and-forth discussions.

@rgbkrk rgbkrk mentioned this pull request May 23, 2018
4 tasks
Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

I really like this solution 👍 Thanks @loganfsmyth 😌

@timneutkens
Copy link
Member

Apparently we didn't have a test for compiling with a custom .babelrc so I've added one.

@timneutkens timneutkens merged commit bb7722f into vercel:canary May 23, 2018
@timneutkens
Copy link
Member

Thank you very much @loganfsmyth 🙌

dcalhoun pushed a commit to dcalhoun/next.js that referenced this pull request May 24, 2018
…er and loader options (vercel#4417)

This resolves the

> .value is not a valid Plugin property

error showing up for people in vercel#4227

cc @timneutkens
lependu pushed a commit to lependu/next.js that referenced this pull request Jun 19, 2018
…er and loader options (vercel#4417)

This resolves the

> .value is not a valid Plugin property

error showing up for people in vercel#4227

cc @timneutkens
@lock lock bot locked as resolved and limited conversation to collaborators May 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants