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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃彈馃殌 Provide a way to synchronously transform CSS in Babel #32485

Merged
merged 2 commits into from
Feb 8, 2021
Merged

馃彈馃殌 Provide a way to synchronously transform CSS in Babel #32485

merged 2 commits into from
Feb 8, 2021

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Feb 7, 2021 •
edited

Background:

#30206 added a babel plugin for JSS transforms via postcss. Since babel is sync but postcss plugins are async, the PR used a child_process hack. See #30206 (comment) for the gory details. Under a profiler, each transform takes 2-3s to execute.

PR highlights:

This PR adds a sync-rpc wrapper to the transform function. It too uses a child_process, but the overhead is paid for just the first call. Every subsequent transform executes in < 50ms. Gains will be higher as the number of JSS transforms increases.

Time spent by gulp dist on transforms:

Before: 8%.

image

After: 1%.

image

Addresses #30206 (comment)

@rsimha rsimha self-assigned this Feb 7, 2021
@rsimha rsimha requested review from samouri, dvoytenko and jridgewell and removed request for rileyajones February 7, 2021 19:30
@rsimha rsimha marked this pull request as ready for review February 7, 2021 19:30
@amp-owners-bot
Copy link

amp-owners-bot bot commented Feb 7, 2021

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

build-system/babel-plugins/babel-plugin-transform-jss/index.js
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/ignore-unrelated-exports/output.mjs
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/should-transform-jss-var/output.mjs
build-system/babel-plugins/babel-plugin-transform-jss/test/fixtures/transform-assertions/should-transform-literal/output.mjs

@rsimha rsimha merged commit e28f933 into ampproject:master Feb 8, 2021
@rsimha rsimha deleted the 2021-02-06-TransformCss branch February 8, 2021 19:56
samouri
samouri reviewed Feb 9, 2021
View reviewed changes
Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

馃敟 馃敟 馃敟 馃敟 馃敟
AMAZING!
馃敟 馃敟 馃敟 馃敟 馃敟

FMI: what are you using to profile?

@rsimha
Copy link
Contributor Author

rsimha commented Feb 9, 2021

FMI: what are you using to profile?

npx 0x -o node_modules/gulp/bin/gulp.js dist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samouri samouri samouri left review comments

@dvoytenko dvoytenko dvoytenko approved these changes

@jridgewell jridgewell Awaiting requested review from jridgewell

Assignees

@rsimha rsimha

Labels
PR use: In Beta / Experimental PR use: In LTS PR use: In Stable WG: infra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rsimha @dvoytenko @samouri @ampprojectbot