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

🏗️🚀 Babel compile with sourcemaps #27447

Merged
merged 6 commits into from Mar 28, 2020
Merged

🏗️🚀 Babel compile with sourcemaps #27447

merged 6 commits into from Mar 28, 2020

Conversation

jridgewell
Copy link
Contributor

Fixes the sourcemap generation for our pre-closure babel compile.

Additionally, replaces our in-memory cache with a persistent disk cache, so re-compiling will be even faster:

$ npx gulp dist --core_runtime_only --noextensions
...snip...
[00:10:11] Finished 'dist' after 1.08 min

$ npx gulp dist --core_runtime_only --noextensions
...snip...
[00:11:03] Finished 'dist' after 26 s

@jridgewell jridgewell requested a review from rsimha March 27, 2020 04:31
@amp-owners-bot
Copy link

Hey @erwinmombay, these files were changed:

build-system/compile/single-pass.js

@jridgewell jridgewell force-pushed the precompile-sourcemaps branch 2 times, most recently from d6966a4 to 9b7c9d6 Compare March 27, 2020 07:26
rsimha
rsimha suggested changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment •
edited

Choose a reason for hiding this comment

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

I like this PR! Thanks for working on it.

However, it substantially increases the running time of gulp dist.

Macbook:
With #27426: 6:30
With this PR: 8:55

Travis:
With #27426: 5:45
With this PR: 7:25

Maybe addressing #27447 (comment) will improve things?

build-system/compile/pre-closure-babel.js Show resolved Hide resolved
build-system/compile/pre-closure-babel.js Outdated Show resolved Hide resolved
build-system/compile/pre-closure-babel.js Outdated Show resolved Hide resolved
build-system/tasks/clean.js Outdated Show resolved Hide resolved
@rsimha
Copy link
Contributor

rsimha commented Mar 27, 2020 •
edited

I tried moving all the initialization code out of preClosureBabel(), and still got a running time of 9 minutes for gulp dist on my Macbook (up from 6:30). Looks like the move from an in-memory cache to an on-disk cache is really slowing down clean builds, even if it does speed up subsequent builds. We can't check this in as-is because all our Travis builds are clean builds.

I think we should make just the sourcemaps fixes in this PR, and independently try out gulp-babel and gulp-cache in two separate future PRs to see what the time differences are for each of them. WDYT?

jridgewell
jridgewell commented Mar 27, 2020
View reviewed changes
Copy link
Contributor Author

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

I think we should make just the sourcemaps fixes in this PR, and independently try out gulp-babel and gulp-cache in two separate future PRs to see what the time differences are for each of them. WDYT?

gulp-babel is necessary to support sourcemaps. It's essentially a wrapper around babel.transformFile with sourcemap soupport.

But as I used gulp-cache more, I started to dislike it. I'll remove.

build-system/compile/pre-closure-babel.js Outdated Show resolved Hide resolved
build-system/compile/pre-closure-babel.js Show resolved Hide resolved
build-system/compile/pre-closure-babel.js Outdated Show resolved Hide resolved
build-system/tasks/clean.js Outdated Show resolved Hide resolved
@jridgewell
Babel compile with sourcemaps
1607671 
Fixes the sourcemap generation for our pre-closure babel compile.

Aditionally, replaces our in-memory cache with a persistent disk cache, so re-compiling will be even faster:

```bash
$ npx gulp dist --core_runtime_only --noextensions
...snip...
[00:10:11] Finished 'dist' after 1.08 min

$ npx gulp dist --core_runtime_only --noextensions
...snip...
[00:11:03] Finished 'dist' after 26 s
```
@jridgewell
Avoid gulp-if
cb23515 
It was giving sooo many problems.
@jridgewell
Don't bust cache just because there's a new file
3ea4d59
@jridgewell
Remove gulp-cache use
1f22f84
@jridgewell
Don't comment out experiments
1d6565c
rsimha
rsimha approved these changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment •
edited

Choose a reason for hiding this comment

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

gulp-babel is necessary to support sourcemaps. It's essentially a wrapper around babel.transformFile with sourcemap soupport.

Gotcha. Definitely cleaner now, and it's back to being fast. LGTM after you update the PR description.

build-system/compile/pre-closure-babel.js Show resolved Hide resolved
@rsimha
Copy link
Contributor

rsimha commented Mar 27, 2020 •
edited

Oh, don't forget to fix single pass too

@jridgewell
Copy link
Contributor Author

It's already fixed: https://github.com/ampproject/amphtml/pull/27447/files#diff-69b4ced885f765149f7d79017937928bR689

@rsimha
Copy link
Contributor

rsimha commented Mar 27, 2020

It's already fixed: #27447 (files)

Sheesh. Sorry, and thank you!

rsimha
rsimha approved these changes Mar 28, 2020
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM. 👍

@jridgewell jridgewell merged commit bcde1ec into ampproject:master Mar 28, 2020
@jridgewell jridgewell deleted the precompile-sourcemaps branch March 28, 2020 01:55
@rsimha rsimha mentioned this pull request Mar 28, 2020
Merged
calebcordry added a commit that referenced this pull request Apr 27, 2020
@calebcordry
Revert "🏗️🚀 Babel compile with sourcemaps (#27447)"
097955b 
This reverts commit bcde1ec.
@rsimha rsimha mentioned this pull request Apr 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsimha rsimha rsimha approved these changes

Assignees
No one assigned
Labels
cla: yes PR use: In Beta / Experimental
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jridgewell @rsimha @googlebot @ampprojectbot