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

🏗 Remove almost all gulp streaming from the compilation pipeline #32903

Merged
merged 6 commits into from Feb 25, 2021
Merged

🏗 Remove almost all gulp streaming from the compilation pipeline #32903

merged 6 commits into from Feb 25, 2021

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Feb 25, 2021

This is another in a series of PRs that modernize our development tasks.

PR highlights:

  • Rewrite css/index.js and dist.js without file streaming
  • Make all file I/O operations asynchronous
  • Some opportunistic clean up (clean up comments / JSDoc, move copyCss() to css/, etc.)
  • Delete gulp-file and gulp-watch from package.json

I've verified that the build/ and dist/ output directories (including 3p and tools) are byte-for-byte identical with the same file structure before and after this PR.

After this, the last remaining item is to get rid of all the file streaming in build-system/compile/compile.js.

Partial fix for #32585

@rsimha rsimha self-assigned this Feb 25, 2021
@rsimha rsimha added this to In progress in `wg-infra` Fixit Week 2/2021 via automation Feb 25, 2021
@rsimha rsimha marked this pull request as ready for review February 25, 2021 13:42
@rsimha
Copy link
Contributor Author

rsimha commented Feb 25, 2021

I diffed all the build/ and dist/ output dirs (including 3p and tools), they’re unchanged by this PR.

Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

You're on 🔥 🔥 🔥 🔥 !

build-system/tasks/css/index.js Show resolved Hide resolved
build-system/tasks/dist.js Show resolved Hide resolved
build-system/tasks/dist.js Show resolved Hide resolved
build-system/tasks/dist.js Outdated Show resolved Hide resolved
build-system/tasks/dist.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@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 review comments! This time, you're getting a bunch of replies with no accompanying code changes. Lemme know if you think there's any code that must be changed here.

build-system/tasks/css/index.js Show resolved Hide resolved
build-system/tasks/dist.js Show resolved Hide resolved
build-system/tasks/dist.js Show resolved Hide resolved
build-system/tasks/dist.js Outdated Show resolved Hide resolved
build-system/tasks/dist.js Outdated Show resolved Hide resolved
`wg-infra` Fixit Week 2/2021 automation moved this from In progress to Reviewer approved Feb 25, 2021
Copy link
Member

@samouri samouri 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 addressing all my questions

build-system/tasks/css/index.js Show resolved Hide resolved
build-system/tasks/css/index.js Outdated Show resolved Hide resolved
@rsimha rsimha merged commit bcee2d5 into ampproject:master Feb 25, 2021
`wg-infra` Fixit Week 2/2021 automation moved this from Reviewer approved to Done Feb 25, 2021
@rsimha rsimha deleted the 2021-02-24-GulpSrc branch February 25, 2021 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants