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

Add a new gulp task for patchWebAnimations #13238

Merged
merged 2 commits into from
Feb 2, 2018
Merged

Add a new gulp task for patchWebAnimations #13238

merged 2 commits into from
Feb 2, 2018

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Feb 2, 2018

#13199 moved the call to patchWebAnimations() from the global scope in gulpfile.js to performBuild(), which is called by build() and watch(). Then, #13221 added a call to dist(). Turns out we need the call in gulp dep-check as well.

This PR adds a new gulp patch-web-animations task that's used as a prerequisite for gulp, gulp build, gulp watch, gulp dist, and gulp dep-check

See https://travis-ci.org/ampproject/amphtml/jobs/336675870#L1736 for the failure case.

Follow up to #13199 and #13221
Fixes #13177
Partial fix for #13227

@rsimha rsimha self-assigned this Feb 2, 2018
@rsimha
Copy link
Contributor Author

rsimha commented Feb 2, 2018

/to @choumx

Tested locally by deleting node_modules and running gulp clean && gulp dep-check. It used to fail, but now passes.

Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

Does dep-check run compileCss()? I don't see the reference in dep-check.js.

@rsimha
Copy link
Contributor Author

rsimha commented Feb 2, 2018

Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

Ah, thanks. Might be more readable to import patchWebAnimations directly into dep-check? I think web animations semantically are not related to CSS.

@rsimha rsimha changed the title Move patchWebAnimations to compileCss, so it's available to all gulp … Add a call to patchWebAnimations to gulp dep-check Feb 2, 2018
@rsimha rsimha changed the title Add a call to patchWebAnimations to gulp dep-check Add a new gulp task for patchWebAnimations Feb 2, 2018
@rsimha
Copy link
Contributor Author

rsimha commented Feb 2, 2018

Good point. I've moved the function into its own gulp task and used it as a dependency wherever it's needed. Much cleaner now.

@rsimha
Copy link
Contributor Author

rsimha commented Feb 2, 2018

/cc @erwinmombay FYI

@rsimha rsimha merged commit 8d6f27e into ampproject:master Feb 2, 2018
@rsimha rsimha deleted the 2018-02-02-WebAnimations branch February 2, 2018 20:49
protonate pushed a commit to protonate/amphtml that referenced this pull request Feb 26, 2018
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gulp occasionally fails because it can't find web-animations-js/web-animations.install
4 participants