Skip to content

Conversation

@nicholasio
Copy link
Member

@nicholasio nicholasio commented Jan 23, 2023

This PR adds support for linaria, which is commonly used to share styled react components between front-end and backend (Gutenberg).

Description of the Change

Instructs 10up-toolkit on how to compile Linaria code.

Alternate Designs

Possible Drawbacks

Had to use my fork of the react-fast-refresh-webpack plugin until pmmmwh/react-refresh-webpack-plugin#726 gets merged.

Verification Process

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.
  • I have added a changeset to my PR. See CONTRIBUTING document for instructions

@changeset-bot
Copy link

changeset-bot bot commented Jan 23, 2023

🦋 Changeset detected

Latest commit: 19cf806

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
10up-toolkit Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@nicholasio
Copy link
Member Author

This is blocked by pmmmwh/react-refresh-webpack-plugin#726

use: [
require.resolve('thread-loader'),
{
loader: require.resolve('./plugins/noop-loader'),
Copy link
Member Author

@nicholasio nicholasio Feb 17, 2023

Choose a reason for hiding this comment

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

thread-loader is incompatible with linaria (apparently there's a way but don't feel like it's worth it). I added a noop-loader just in case projects are manipulating rules and if simply remove we might introduce off-by-one errors.

I never really understood the benefits of thread-loader so I'm not concerned about us just ditching it.... I believe this is a relatively safe change that shouldn't be breaking but open to your thoughts @Antonio-Laguna

Copy link
Member

Choose a reason for hiding this comment

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

I think you're correct! I don't believe this is breaking! The only effect is that it might (only might) reduce speed by a tiny bit given there are no extra workers.

@nicholasio nicholasio marked this pull request as ready for review February 17, 2023 14:48
@nicholasio
Copy link
Member Author

nicholasio commented Feb 17, 2023

not sure why tests are failing

UPDATE: fixed.

Antonio-Laguna
Antonio-Laguna previously approved these changes Feb 21, 2023
Copy link
Member

@Antonio-Laguna Antonio-Laguna left a comment

Choose a reason for hiding this comment

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

This LGTM!

I've added some minor nitpick formatting with casing and such. I did double-check that on their repo's README they always capitalize. Not a blocker by any means!

use: [
require.resolve('thread-loader'),
{
loader: require.resolve('./plugins/noop-loader'),
Copy link
Member

Choose a reason for hiding this comment

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

I think you're correct! I don't believe this is breaking! The only effect is that it might (only might) reduce speed by a tiny bit given there are no extra workers.

"@babel/core": "^7.17.8",
"@babel/eslint-parser": "^7.17.0",
"@pmmmwh/react-refresh-webpack-plugin": "^0.5.5",
"@pmmmwh/react-refresh-webpack-plugin": "https://github.com/nicholasio/react-refresh-webpack-plugin.git",
Copy link
Member

Choose a reason for hiding this comment

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

Is this fork related to this change? I'm mainly curious

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - hopefully we can go back to upstream once this is merged and released pmmmwh/react-refresh-webpack-plugin#726

@nicholasio nicholasio merged commit 799afd5 into develop Feb 22, 2023
@nicholasio nicholasio deleted the feature/linaria branch February 22, 2023 01:43
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.

2 participants