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

✨ Gulp task to convert analytic vendor JS configs to JSON #22769

Merged
merged 10 commits into from Jun 24, 2019

Conversation

jonathantyng-amp
Copy link
Contributor

Adds a gulp task to convert the analytic vendor JS files to JSON files.

cc @zhouyx for review

@jonathantyng-amp
Copy link
Contributor Author

cc @zhouyx for review!

build-system/tasks/generate-vendor-jsons.js Outdated Show resolved Hide resolved
build-system/tasks/generate-vendor-jsons.js Show resolved Hide resolved
@rsimha
Copy link
Contributor

rsimha commented Jun 24, 2019

@jonathantyng-amp, before I do a full review, could you help me understand what this task does, how long it takes, and when you typically run it? (Development? Release? What about minified vs. unminified builds?)

Our browserify configs are currently fragmented (I'm in the process of trying to consolidate it), so I'd like to avoid further fragmentation if possible.

@jonathantyng-amp
Copy link
Contributor Author

@rsimha
Context: amp-analytics uses ~70 vendor configs that are currently all compiled into the bundle. We want to separate these out to lazy load them instead. As part of this process we want to convert each vendor config into its own JSON file.

This gulp task converts the current vendor configs into their own JSON file, placing the files into /extensions/amp-analytics/0.1/vendors/. This gulp task will only be run in the next several weeks temporarily while we run some AB tests to make sure the vendor configs stay in sync. After the AB tests are complete, we will permanently commit the JSON files and this gulp task will not be needed anymore.

This task will be run in the release process and runs in ~3 sec

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.

Thanks for the detailed explanation! Since this is temporary, the code LGTM as written.

@zhouyx zhouyx merged commit 526eee1 into ampproject:master Jun 24, 2019
@zhouyx
Copy link
Contributor

zhouyx commented Jun 24, 2019

🎉 Merged

thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
…#22769)

* add gulp task to generate vendor jsons

* finish adding gulp task to convert vendor configs

* also generate _fake_.json

* add moat canary json

* change json spacing to 2

* add comment

* add file to forbidden term whitelist

* remove unecessary moat canary JSON
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

4 participants