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

Update sass to work around CRA build crashes #2643

Merged
merged 1 commit into from
Jan 22, 2020
Merged

Conversation

BPScott
Copy link
Member

@BPScott BPScott commented Jan 21, 2020

WHY are these changes introduced?

Fixes #2629

The current version of CRA' s build uses a version of https://github.com/postcss/postcss-custom-properties that crashes when it comes across var()s inside calc()s a very specific order of calls: top:calc(var(--control-border-width)*-1). See #2629 (comment) for more info. This has been fixed in the newest version of postcss-custom-properties but that's not made its way into CRA (via postcss-preset-env) and probably won't be for some time.

WHAT is this pull request doing?

Flip the order of values (yes I know this sounds silly)

How to 🎩

Ask @dleroux nicely to see that there are no visual regressions.

  • Do a build yarn run build
  • Create a new CRA project with yarn create react-app testing
  • Copy the build styles.css into the CRA app and import the styles from CRA's src/ index.js file and see if a build compiles using yarn run build

@github-actions
Copy link
Contributor

github-actions bot commented Jan 21, 2020

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

No significant changes to src/**/*.tsx were detected.


This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

@dleroux
Copy link
Contributor

dleroux commented Jan 22, 2020

It looks like this is working Ben. The same approach is being used here: https://github.com/Shopify/polaris-react/blob/master/src/styles/foundation/_focus-ring.scss#L31 I assume this one will need to change as well?

@BPScott
Copy link
Member Author

BPScott commented Jan 22, 2020

The same approach is being used here: https://github.com/Shopify/polaris-react/blob/master/src/styles/foundation/_focus-ring.scss#L31 I assume this one will need to change as well?

Funky. That's been in there for a while and hasn't caused crashes. Along with a bunch of other places, searching our codebase for calc(var(.

I've done some digging and it's not calc(var()) that's the problem and now I'm mad. #2629 (comment)

Changing calc(var(--control-border-width) * -1); to calc(-1 * var(--control-border-width)); should get this to compile.

@BPScott BPScott changed the title Use sass variables for some math Update sass to work around CRA build crashes Jan 22, 2020
@BPScott BPScott force-pushed the avoid-cra-crash branch 2 times, most recently from 591e7a3 to e12b31e Compare January 22, 2020 18:57
This fixes build crashes in CRA thanks to a parser bug that explodes
when it encounters the original order (but inverted is fine)
@BPScott BPScott merged commit a3fcf46 into master Jan 22, 2020
@BPScott BPScott deleted the avoid-cra-crash branch January 22, 2020 19:30
@BPScott BPScott mentioned this pull request Feb 19, 2020
6 tasks
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.

Failed to compile ./node_modules/@shopify/polaris/styles.css
2 participants