bento jss: transform the CSS and useStyles exports. #29777

samouri · 2020-08-11T00:27:56Z

summary
Creates a new babel transform that compiles the jss file for amp modes. It does three things:

Replaces useStyles with a side-effect free version that simply returns an object.
Replaces the CSS export with a compiled string for inclusion in shadow.
Removes the import from react-jss, since it is no longer used. Ideally it would have been removed via treeshaking, but I don't trust it fully due to module side-effects.

For: #29862
Related to ampproject/wg-bento#7, ampproject/wg-bento#12, ampproject/wg-bento#13, ampproject/wg-bento#15, ampproject/wg-bento#15

samouri · 2020-08-21T23:54:49Z

hack alert
There was one issue I ran into that I couldn't figure out an elegant way around.
Notably that in order to transform the jss, we need to be able to extract the evaluated jss object from the file. The only way I could figure out to do this was:

Write the .jss.js file using CommonJS exports, and no es6+ features.
Within the babel plugin, extract the jss via require(state.file.opts.filename).JSS. We can't require esm, which is why I wrote it using cjs.
Within the babel plugin, transform the cjs export into esm named exports, otherwise Closure Compiler panics with JSC_DOES_NOT_HAVE_EXPORT.

@jridgewell: wdyt about this approach? a cleaner (but slower) method would be to first run the .jss.js file through a one-off babel pass and write it to a tempfile which we could then require. Is that preferable? Would that get tricky if we wanted to support watching since I'd need to kill the module cache.

samouri · 2020-08-21T23:55:44Z

Woot! After discussing with @jridgewell he informed me about path.evaluate(), which does exactly what we want. It allows us to statically retrieve the value of JSS without any extra compilation to tmp files and using require.

We just can't get too fancy with our .jss.js files, which we wouldn't have wanted anyway. Hack is solved :)

amp-owners-bot · 2020-08-21T23:58:08Z

Hey @erwinmombay, @jridgewell! These files were changed:

build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/argument-function/output.js
build-system/babel-plugins/babel-plugin-transform-jss/index.js
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/ignore-non-jss-file/input.js
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/ignore-non-jss-file/options.json
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/ignore-non-jss-file/output.mjs
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/should-throw-classname/input.js
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/should-throw-classname/options.json
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/should-throw-confidence/input.js
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/should-throw-confidence/options.json
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/should-transform-jss-var/input.js
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/should-transform-jss-var/options.json
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/should-transform-jss-var/output.mjs
+4 more

Hey @wassgha! These files were changed:

build-system/tasks/storybook/preact-env/webpack.config.js

dvoytenko

This looks really nice!

...ns/babel-plugin-transform-jss/test/fixtures/transform-assertions/should-transform/output.mjs

build-system/babel-plugins/babel-plugin-transform-jss/index.js

...gins/babel-plugin-transform-jss/test/fixtures/transform-assertions/should-transform/input.js

dvoytenko · 2020-08-24T17:06:59Z

test/manual/amp-base-carousel/basic.amp.html

  <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
  <link href='https://fonts.googleapis.com/css?family=Questrial' rel='stylesheet' type='text/css'>
-  <script async custom-element="amp-base-carousel" src="https://cdn.ampproject.org/v0/amp-base-carousel-0.1.js"></script>
+  <script async custom-element="amp-base-carousel" src="https://cdn.ampproject.org/v0/amp-base-carousel-1.0.js"></script>


The 0.1 might be needed for some particular tests, especially since 0.1 is still the PROD version. Can we test via storybook? I like those a lot better now.

err, yeah I didn't mean to include this. will revert.
Do you even use storybook to test bento components in amp mode?

We do. However, there's a known issue there that I'm fixing right now. See ampproject/storybook-addon-amp#2

dvoytenko

Approved, but I think there are issues with CSS string

...-plugin-transform-jss/test/fixtures/transform-assertions/should-transform-jss-var/output.mjs

build-system/babel-plugins/babel-plugin-transform-jss/index.js

jridgewell · 2020-08-25T19:31:29Z

...-plugin-transform-jss/test/fixtures/transform-assertions/should-transform-jss-var/output.mjs

@@ -0,0 +1,23 @@
+var _classes = JSON.parse("{\"button\":\"button-0-2-1\",\"CSS\":\".button-0-2-1 {\\n  font-size: 12px;\\n}\"}");


Can we minify this CSS at all?

Yes, but I'd like to defer that

I hope that eventually we can run our CSS optimizer/prefixer code on these stylesheets.

google-cla · 2020-08-26T15:13:33Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-08-26T15:42:56Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-08-26T15:43:04Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-08-26T16:09:49Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

dvoytenko · 2020-08-26T21:18:12Z

\o/\o/\o/

dvoytenko · 2020-08-26T21:21:07Z

@samouri Please tag the ITI that you created to this pull request.

samouri · 2020-08-26T22:47:55Z

Sure, here's the I2I #29862.

We still have a couple of things to iron out before this story is complete:

transformations (jss plugins, our custom optimizations, minifications)
distribution format. do we want a single option, or multiple (jss peer dep vs. distribute a .css file vs. both)

* build-system: teach buildExtension how to build jss * remove style-loader from main deps * make work with watch mode * add babel transform * delete jss build, perform at babel time * lint * dd todo * also convert the module.exports into named exports * copyright * Thank you justin, path.evaluate is gold. * Update with smarter xform * extract classes var, no longer jss required export * another test * remove rest operator * fix, bad interpolation * improve two comments * lint * updates, tanks justin * lint * Update build-system/babel-plugins/babel-plugin-transform-jss/index.js Co-authored-by: Justin Ridgewell <[email protected]> * Update build-system/babel-plugins/babel-plugin-transform-jss/index.js Co-authored-by: Justin Ridgewell <[email protected]> * remove outdated option * lint * remove computer properties for browserify * Allow JSON.parse in jss files for conformance-config * test code also needs transform Co-authored-by: Justin Ridgewell <[email protected]>

google-cla bot added the cla: yes label Aug 11, 2020

samouri changed the title ~~Jss3~~ bento jss: add transform for amp mode. Aug 11, 2020

samouri force-pushed the jss3 branch 3 times, most recently from c36b42c to 3cd59ae Compare August 18, 2020 17:28

samouri changed the title ~~bento jss: add transform for amp mode.~~ bento jss: compile at build time Aug 18, 2020

samouri changed the title ~~bento jss: compile at build time~~ bento jss: babel transform the CSS and useStyles exports. Aug 20, 2020

samouri changed the title ~~bento jss: babel transform the CSS and useStyles exports.~~ bento jss: transform the CSS and useStyles exports. Aug 20, 2020

samouri force-pushed the jss3 branch from 208da03 to 0004440 Compare August 21, 2020 23:56

samouri marked this pull request as ready for review August 21, 2020 23:57

amp-owners-bot bot requested a review from nainar August 21, 2020 23:58

samouri mentioned this pull request Aug 22, 2020

amp-base-carousel: control, snap, snap-align, and mixed-length #29932

Closed

dvoytenko suggested changes Aug 24, 2020

View reviewed changes

samouri force-pushed the jss3 branch from 84b407b to f0cee98 Compare August 24, 2020 20:23

samouri requested a review from dvoytenko August 24, 2020 21:16

dvoytenko approved these changes Aug 24, 2020

View reviewed changes

...-plugin-transform-jss/test/fixtures/transform-assertions/should-transform-jss-var/output.mjs Outdated Show resolved Hide resolved

jridgewell reviewed Aug 25, 2020

View reviewed changes

jridgewell approved these changes Aug 25, 2020

View reviewed changes

samouri added 8 commits August 26, 2020 10:15

build-system: teach buildExtension how to build jss

da4536b

remove style-loader from main deps

da50fa7

make work with watch mode

e20a201

add babel transform

cbee488

delete jss build, perform at babel time

98a32a2

lint

27604ab

dd todo

dc7206f

also convert the module.exports into named exports

b571ede

remove computer properties for browserify

93a66cd

samouri force-pushed the jss3 branch from 3163208 to 93a66cd Compare August 26, 2020 15:12

google-cla bot added cla: no and removed cla: yes labels Aug 26, 2020

samouri added cla: yes and removed cla: no labels Aug 26, 2020

Allow JSON.parse in jss files for conformance-config

8eacdcb

samouri force-pushed the jss3 branch from f2190af to 8eacdcb Compare August 26, 2020 15:42

google-cla bot added cla: no and removed cla: yes labels Aug 26, 2020

samouri added cla: yes and removed cla: no labels Aug 26, 2020

test code also needs transform

32543e5

google-cla bot added cla: no and removed cla: yes labels Aug 26, 2020

samouri self-assigned this Aug 26, 2020

samouri added cla: yes and removed cla: no labels Aug 26, 2020

samouri merged commit 6f97154 into ampproject:master Aug 26, 2020

samouri deleted the jss3 branch August 26, 2020 18:17

samouri mentioned this pull request Aug 28, 2020

wg-runtime Status Update 2020-08-31 ampproject/wg-runtime#42

Closed

ampprojectbot added the PR use: In Beta / Experimental label Sep 2, 2020

		@@ -0,0 +1,23 @@
		var _classes = JSON.parse("{\"button\":\"button-0-2-1\",\"CSS\":\".button-0-2-1 {\\n font-size: 12px;\\n}\"}");

bento jss: transform the CSS and useStyles exports. #29777

bento jss: transform the CSS and useStyles exports. #29777

Uh oh!

Conversation

samouri commented Aug 11, 2020 • edited by dvoytenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samouri commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samouri commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amp-owners-bot bot commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvoytenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dvoytenko Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

samouri Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

dvoytenko Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

dvoytenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jridgewell Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

samouri Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

dvoytenko Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

google-cla bot commented Aug 26, 2020

Uh oh!

google-cla bot commented Aug 26, 2020

Uh oh!

google-cla bot commented Aug 26, 2020

Uh oh!

google-cla bot commented Aug 26, 2020

Uh oh!

dvoytenko commented Aug 26, 2020

Uh oh!

dvoytenko commented Aug 26, 2020

Uh oh!

samouri commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

samouri commented Aug 11, 2020 •

edited by dvoytenko

Loading

samouri commented Aug 21, 2020 •

edited

Loading

samouri commented Aug 21, 2020 •

edited

Loading

amp-owners-bot bot commented Aug 21, 2020 •

edited

Loading

samouri commented Aug 26, 2020 •

edited

Loading