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

[Core] Convert custom webpack cell-loader into Babel Plugin #512

Merged
merged 6 commits into from
May 9, 2020

Conversation

RobertBroersma
Copy link
Contributor

This implements #503 and opens up the possibility for using Cells in Jest!

I've never written a Babel Plugin or done anything with AST's before, so I just wanted to get this out here to gather some feedback before looking into writing tests for it, and also to discuss on where to locate the plugin file!

This also fixes some issues with Cells where they break if you comment out some const exports and supports using function declarations instead of variable function declarations!

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

I actually think this is perfect, but I also don't have a bunch of experience with ASTs. @mojombo did the last implementations for our eslint, router, and cell plugins.

I've made a few suggestions for comments and variable names that reduce the abstractness for the reader of this code.

packages/core/config/babel-plugin-redwood-cell.js Outdated Show resolved Hide resolved
packages/core/config/babel-plugin-redwood-cell.js Outdated Show resolved Hide resolved
RobertBroersma and others added 4 commits May 6, 2020 13:38
Added some documentation and improved readability

Co-authored-by: Peter Pistorius <peter.pistorius@gmail.com>
@RobertBroersma RobertBroersma marked this pull request as ready for review May 6, 2020 14:59
@RobertBroersma
Copy link
Contributor Author

@peterp Thanks for the feedback! It's processed. 👍

I've decided to name the variable what it is; EXPECTED_EXPORTS_FROM_CELL. Let me know if that doesn't make sense.

I also added some tests for the plugin. I noticed tests weren't running on the core package. Is there a reason for that? I configured them to run so I could run my plugin test.

packages/core/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@peterp peterp 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 amazing work, thank you so much Robert!

@RobertBroersma RobertBroersma requested a review from peterp May 6, 2020 17:27
@peterp peterp merged commit 8f11531 into redwoodjs:master May 9, 2020
@thedavidprice thedavidprice added this to the next release milestone May 9, 2020
@thedavidprice
Copy link
Contributor

@RobertBroersma great work all around here, Robert, from super helpful inline comments to ramping up tests for the core package! 🚀

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.

None yet

3 participants