Amphtml: gulp watch and rebuild on save sometimes break

[gmajoulet]

On MacOS, running the gulp command sometimes don't rebuild on file update. I've never had the issue on Linux though. Restarting gulp usually fixes the issue.

To repro:

1. Run gulp (I was using the --extensions_from param)
2. Update a file
3. BAD: sometimes it doesn't rebuild the file

It either works or doesn't right after gulp started running. It doesn't work for some time and then stop working.

cc @ampproject/wg-infra

[rsimha]

@gmajoulet A few races in the lazy-build logic of gulp were recently fixed (to make the server correctly wait for pending compilations). Can you post a screenshot of the logs when you see this?

As a sanity check, files are recompiled on edit only after they are initially served (or if they are included in the list of files to pre-compile, via --extensions or --extensions_from). Therefore, if you run gulp and load a page that uses amp-foo.js but not amp-bar.js, it is expected behavior that amp-foo.js will be recompiled on edit, but not amp-bar.js.

Curious about the exact behavior you observed.

[rsimha]

Bumping this for more input.

[gmajoulet]

Just repro'd with:

gulp --extensions_from=path/index.html

Where index.html has:

    <script async src="https://cdn.ampproject.org/v0.js"></script>
    <script async custom-element="amp-story"
        src="https://cdn.ampproject.org/v0/amp-story-1.0.js"></script>

    <script async custom-element="amp-video"
        src="https://cdn.ampproject.org/v0/amp-video-0.1.js"></script>
    <script async custom-element="amp-audio"
        src="https://cdn.ampproject.org/v0/amp-audio-0.1.js"></script>
    <script async custom-element="amp-twitter"
        src="https://cdn.ampproject.org/v0/amp-twitter-0.1.js"></script>
    <script async custom-element="amp-youtube"
        src="https://cdn.ampproject.org/v0/amp-youtube-0.1.js"></script>
    <script async custom-element="amp-social-share"
        src="https://cdn.ampproject.org/v0/amp-social-share-0.1.js"></script>
    <script async custom-element="amp-viewer-integration"
        src="https://cdn.ampproject.org/v0/amp-viewer-integration-0.1.js"></script>
    <script async custom-element="amp-sidebar"
        src="https://cdn.ampproject.org/v0/amp-sidebar-0.1.js"></script>
    <script async custom-element="amp-analytics"
        src="https://cdn.ampproject.org/v0/amp-analytics-0.1.js"></script>
    <script async custom-element="amp-story-education"
        src="https://cdn.ampproject.org/v0/amp-story-education-0.1.js"></script>
    <script async custom-element="amp-consent" src="https://cdn.ampproject.org/v0/amp-consent-0.1.js"></script>
    <script async custom-element="amp-geo" src="https://cdn.ampproject.org/v0/amp-geo-0.1.js"></script>

AFTER getting the log

[16:54:36] JS and extensions will be lazily built when requested...
[16:54:36] Finished 'default' after 55 s

I updated amp-story-page.js, which is part of amp-story.js but did not re-build.

[rsimha]

Thanks for the clarification. What you are seeing is definitely a bug. When we initialize the lazy builder for an extension, we watch the entire extension directory, and editing any file inside of it should cause the extension to be rebuilt.

https://github.com/ampproject/amphtml/blob/f1b2406ddfc69c5d1d077ef39ffdea11b298f082/build-system/tasks/extension-helpers.js#L372-L397

I tried reproing the bug by using this equivalent set of steps:

# Keep this running in a terminal window
gulp --extensions amp-story

# Repeatedly run this in a new terminal window
touch extensions/amp-story/1.0/amp-story-page.js

I saw a failure to rebuild amp-story when the second step was run about one in every few attempts. When I added debugging, the watcher was not reliably detecting the file edit.

I suspect the root cause lies in our use of gulp-watch, which uses an old version of chokidar under the covers to watch for file changes. Now that we're on gulp v4 which has its own built-in watcher, I believe this issue can be fixed by migrating away from gulp-watch and using the native watcher in gulp.

I'll send out a PR, after which we can see if this issue rears its head again. Stay tuned.

[gmajoulet]

Thank you for looking into this Raghu! I know these are very hard to debug so I really appreciate your work to make our dev experience better 🙌

[rsimha]

Speculative fix has been merged. Assigning back to @gmajoulet for a few days worth of observation before this can be closed.

[estherkim]

few days worth of observation

Soo.. watching?

[gmajoulet]

Closing this, we can re-open if we see it happening again :))
Thanks!