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.

Sign up for GitHub

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

Jump to bottom

[styles] Upgrade JSS to v10-alpha #14006

Merged
merged 2 commits into from
Dec 29, 2018
Merged

[styles] Upgrade JSS to v10-alpha #14006

merged 2 commits into from
Dec 29, 2018

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Dec 26, 2018 •
edited
Loading

Gain around 10% of speed on the whole spectrum.

-Box x 2,489 ops/sec ±7.37% (71 runs sampled)
+Box x 3,043 ops/sec ±4.14% (179 runs sampled)
EmotionButton x 6,856 ops/sec ±5.06% (165 runs sampled)
hashing x 221,423 ops/sec ±1.22% (190 runs sampled)
-HookButton x 10,386 ops/sec ±4.03% (84 runs sampled)
+HookButton x 11,265 ops/sec ±3.43% (178 runs sampled)
JSS naked x 21,021 ops/sec ±7.27% (72 runs sampled)
JSSButton x 13,573 ops/sec ±5.01% (78 runs sampled)
Naked x 32,683 ops/sec ±3.54% (172 runs sampled)
StyledComponentsButton x 6,745 ops/sec ±2.91% (181 runs sampled)
-WithStylesButton x 7,384 ops/sec ±4.94% (74 runs sampled)
+WithStylesButton x 8,442 ops/sec ±3.31% (176 runs sampled)

@oliviertassinari oliviertassinari force-pushed the jss-v10 branch 2 times, most recently from 5f5fc3a to 12af3de Compare December 26, 2018 17:40
TrySound
TrySound reviewed Dec 27, 2018
View reviewed changes
packages/material-ui-styles/src/jssPreset.js
import jssPropsSort from 'jss-props-sort';
import functions from 'jss-plugin-rule-value-function';
import global from 'jss-plugin-global/dist/jss-plugin-global.cjs';
import nested from 'jss-plugin-nested/dist/jss-plugin-nested.cjs';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is something broken with esm version?

Copy link
Member Author

@oliviertassinari oliviertassinari Dec 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I have faced this exact issue: pmndrs/react-spring#278. Seems to affect Next.js and Gatsby.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which exactly? I see people described a few different issues there.

Copy link
Member Author

@oliviertassinari oliviertassinari Dec 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is an issue with the export syntax.

(function (exports, require, module, __filename, __dirname) { import _extends from '@babel/runtime/helpers/esm/extends';
                                                                     ^^^^^^^^
SyntaxError: Unexpected identifier

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like next.js resolves all imports using bundler which of course makes the output full of imports. The interesting thing is why only these two packages are broken? jss itself has the same setup.

Copy link
Member Author

@oliviertassinari oliviertassinari Dec 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at the linked issue, it might also be an issue with Gatbsy but I haven't spent the time to test that hypothesis. JSS v10, while in alpha, looks good to me from an "output" perspective. It's larger but faster. My main concern is with the upgrade path. We need to make sure it's working with no breaking change. I'm gonna try to scope the update to @material-ui/styles.

@oliviertassinari oliviertassinari force-pushed the jss-v10 branch 3 times, most recently from 6d4d3ac to d71b4d1 Compare December 27, 2018 11:23
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 27, 2018 •
edited
Loading

I'm splitting this pull-requests into different chunks; so we focus on JSS v10 here.

@oliviertassinari oliviertassinari changed the title [core] Upgrade JSS to v10-alpha [styles] Upgrade JSS to v10-alpha Dec 27, 2018
@oliviertassinari oliviertassinari force-pushed the jss-v10 branch 3 times, most recently from 06622dd to d6a99ca Compare December 27, 2018 22:53
@oliviertassinari
Copy link
Member Author

@eps1lon Any idea how we could solve the TypeScript CI fail?

@eps1lon
Copy link
Member

eps1lon commented Dec 28, 2018 •
edited
Loading

Well some are mostly typo changes (JSS -> Jss) but some are also breaking changes at runtime (generateClassName -> generateId).

If we don't want to propagate those to our users we have map them e.g. when handling options in makeStyles. 82c3717 fixes all typescript errors but it shows the breaking changes that we would propagate to our users.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 28, 2018 •
edited
Loading

The visual regression test fail confirms that JSS v10 add support for nested style function 🎉. well done @kof.

capture d ecran 2018-12-28 a 14 42 09

@eps1lon Thank you for the fix! I have tried to isolate the changes to @material-ui/styles. If I understand the situation correctly, the breaking change for @material-ui/core/styles is minimal. Only a fraction of the TypeScript API changes. (It only change because we use a mono repository using a single version of JSS).

@oliviertassinari
Copy link
Member Author

I'm gonna try to better leverage the resolution feature of yarn to remove the TypeScript broken change.

@eps1lon
Copy link
Member

eps1lon commented Dec 28, 2018

The only breaking change is basically that wherever we have options that are passed to jss we create the following breakage:

-{ generateClassName }
+{ generateId: generateClassName }

That applies to @material-ui/core/styles/withStyles ( and JSSProvider I think ) as well as @material-ui/styles/makeStyles and @material-ui/styles/withStyles.

Everything else is just internal.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 28, 2018 •
edited
Loading

I have never seen anyone use the generateClassName option of the withStyles, it might not even work.
In this case, I will focus my energy on another topic.

@oliviertassinari oliviertassinari mentioned this pull request Dec 28, 2018
Closed
@oliviertassinari oliviertassinari merged commit 86a29a9 into mui:master Dec 29, 2018
@oliviertassinari oliviertassinari deleted the jss-v10 branch December 29, 2018 11:26
@oliviertassinari oliviertassinari mentioned this pull request Dec 30, 2018
Closed
2 tasks
eps1lon
eps1lon reviewed Dec 31, 2018
View reviewed changes
package.json
@@ -182,7 +183,8 @@
"workbox-build": "^3.6.3"
},
"resolutions": {
"**/hoist-non-react-statics": "^3.2.1"
"**/hoist-non-react-statics": "^3.2.1",
"jss": "^10.0.0-alpha.3"
Copy link
Member

Choose a reason for hiding this comment

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

@oliviertassinari What was the reason for this? This means we only test with an alpha version of jss not the latest stable.

Copy link
Member Author

@oliviertassinari oliviertassinari Jan 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Without this resolution, yarn will install JSS v10 and v9 at the same time. It breaks at runtime. I wish we had a better alternative.

@oliviertassinari oliviertassinari mentioned this pull request Dec 31, 2018
Closed
2 tasks
@zannager zannager added the package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. label Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eps1lon eps1lon eps1lon left review comments

@TrySound TrySound TrySound left review comments

Assignees
No one assigned
Labels
package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oliviertassinari @eps1lon @TrySound @zannager