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.

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

✨ Update analytics vendor split experiments config #23604

Merged
merged 4 commits into from Jul 31, 2019

Conversation

jonathantyng-amp
Copy link
Contributor

  • Add gulp generate-vendor-configs to experiments command so that vendor JSONs are generated during the experiment build
  • Enable --compile_vendor_configs by default, so no need to add this flag anymore

@jonathantyng-amp
Copy link
Contributor Author

cc @zhouyx for review :)

cc @erwinmombay just want to make sure, is running 2 commands during experiment builds okay? e.g. gulp generate-vendor-jsons && gulp dist --defineExperimentConstant=ANALYTICS_VENDOR_SPLIT

@zhouyx
Copy link
Contributor

zhouyx commented Jul 31, 2019

A few notes on this PR:

  1. Our release script always run gulp generate-vendor-jsons and gulp dist --compile_vendor_configs.
    By adding gulp generate-vendor-jsons to the experiment build command, we will be generating vendor jsons twice. But that should be fine given it only take a few seconds.
    Future idea is that we want to remove the gulp generate-vendor-jsons task from everywhere after launching the experiment.

  2. Remove compile_vendor_configs in this PR. Because the flag itself is easy to be missed. Given in the future all build will need to copy the json file to the dist folder. we decide to remove the flag and make it a default process during build.

  3. The reason we are making the change now is because we want to unblock @estherkim's work on integration test. We figure it's easier to modify the experiment build command instead of rewriting the default build command in travis.

@@ -386,7 +386,7 @@ function buildExtension(
}

// minify and copy vendor configs for amp-analytics component
if (name === 'amp-analytics' && argv.compile_vendor_configs) {
if (name === 'amp-analytics') {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jonathantyng-amp could you please confirm that the build works even if gulp generate-vendor-jsons hasn't been run (no such json files to copy). In that case what will happen? Will the analytics-vendors folder be created? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested locally and confirmed that build works with no JSON files to copy. If there are no vendor JSONs, dist/analytics-vendors folder will not be created

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Thanks

@zhouyx zhouyx merged commit 8af2f65 into ampproject:master Jul 31, 2019
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
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.

None yet

3 participants