Skip to content

add babel transformation to the multipass build#22839

Merged
erwinmombay merged 30 commits intoampproject:masterfrom
erwinmombay:multi-pipeline
Jul 9, 2019
Merged

add babel transformation to the multipass build#22839
erwinmombay merged 30 commits intoampproject:masterfrom
erwinmombay:multi-pipeline

Conversation

@erwinmombay
Copy link
Copy Markdown
Member

@erwinmombay erwinmombay commented Jun 14, 2019

add babel transformation to the multipass build

to accomplish the task we move all the source files to babel preprocess into a temp directory. This is so that we can preprocess only once instead of once per compilation unit (which would turn into an n squared operation)

There were some complications with buildExperiments, buildLoginDone, and buildWebPushPublisherFiles calls since they move they're files into the build directory to be processed by closure way later into the process (in their individual methods) so i had to separate this file move step into a pre${method} invocation that needed to be executed before the babel preprocessing. In buildWebPushPublisherFiles case it also had a post process step.

@amp-bundle-size amp-bundle-size bot requested a review from jridgewell June 14, 2019 18:26
@erwinmombay erwinmombay changed the title add babel transformation to the multipass build [WIP] add babel transformation to the multipass build Jun 24, 2019
@erwinmombay
Copy link
Copy Markdown
Member Author

@jonathantyng-amp @zhouyx

retainLines: true,
});
const name = `${tmp}${file.replace(process.cwd(), '')}`;
fs.outputFileSync(name, code);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sourcemaps, too.

Copy link
Copy Markdown
Member Author

@erwinmombay erwinmombay Jul 2, 2019

Choose a reason for hiding this comment

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

since this is just the preprocess step, we shouldnt need sourcemaps here. we'll still want to rely on CC for the sourcemaps for multipass correct?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You have to combine the source-maps (which closure will do). But to do that, you have to output sourcemaps for CC to consume.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

LMK, if my statement is correct. Also PTAL when you can. Thanks!

Copy link
Copy Markdown
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.

Two preliminary comments. Will do a full review after this is ready.

@erwinmombay erwinmombay force-pushed the multi-pipeline branch 2 times, most recently from 8b2721b to 620c70b Compare July 2, 2019 15:46
@erwinmombay erwinmombay changed the title [WIP] add babel transformation to the multipass build add babel transformation to the multipass build Jul 2, 2019
@erwinmombay erwinmombay requested review from jridgewell and rsimha July 2, 2019 16:14
@erwinmombay
Copy link
Copy Markdown
Member Author

@rsimha @jridgewell this is ready for review

@erwinmombay erwinmombay force-pushed the multi-pipeline branch 2 times, most recently from 13e3ae0 to 4f5cb26 Compare July 2, 2019 17:13
Copy link
Copy Markdown
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.

A few comments below. Will review again once this is rebased.

@erwinmombay
Copy link
Copy Markdown
Member Author

i had to leave the assert/log transformation in java/closure compiler for now. (I'll follow up in a separate PR)

There is currently a weird interaction where when we do the transform in babel when we leave a bare "string", Closure Compiler does not remove the dead string expression statements. We cannot just outright remove the argument of the assert/log calls since we would need to inspect if the arguments have any method calls (which might have side effects).

@erwinmombay
Copy link
Copy Markdown
Member Author

@rsimha Please take another look. Thanks!

@erwinmombay
Copy link
Copy Markdown
Member Author

some additional notes:

  • +/- 30 to 40s increase in gulp dist
  • diff of v0.js, code is almost exactly the same except for the optimization done for the template literals
  • number of files is exactly the same
  • build login html page exactly the same
  • web push publisher files exactly the same
  • experiments page and js file exactly the same

Copy link
Copy Markdown
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.

This is looking good! A few comments below.

@erwinmombay
Copy link
Copy Markdown
Member Author

@rsimha resolved last round of comments. PTAL

@erwinmombay erwinmombay force-pushed the multi-pipeline branch 3 times, most recently from 7dd1b80 to ff7503e Compare July 9, 2019 21:46
@erwinmombay erwinmombay requested a review from rsimha July 9, 2019 22:17
Copy link
Copy Markdown
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.

Nice work!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants