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

🏗♻️ Split gulpfile.js into separate files #22109

Merged
merged 3 commits into from
May 2, 2019
Merged

🏗♻️ Split gulpfile.js into separate files #22109

merged 3 commits into from
May 2, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented May 2, 2019

This PR splits up gulpfile.js (~1700 lines) into separate files for easy maintenance, and to pave the way for a long-awaited upgrade to gulp v4.

Since Github's web UI doesn't highlight code that was moved across files but is otherwise unchanged, you can check out this branch and run git diff HEAD^ HEAD --color-moved. The only new code you should see are require and export statements.

Coming up: Upgrade to gulp v4

Copy link
Member

@danielrozenberg danielrozenberg left a comment

Choose a reason for hiding this comment

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

It's difficult to review this file because I can't tell what are changes and what are just direct moves from gulpfile.js into the separate files. Would you mind telling us if there are any places where you made changes that aren't just moves and new/replaced imports?

build-system/tasks/helpers.js Show resolved Hide resolved
build-system/tasks/helpers.js Show resolved Hide resolved
@rsimha
Copy link
Contributor Author

rsimha commented May 2, 2019

@danielrozenberg Re: seeing code that has moved, read PR description.

Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Left a few nits, as requested they are all in one review this time.

build-system/tasks/nailgun.js Show resolved Hide resolved
build-system/tasks/build.js Show resolved Hide resolved
build-system/tasks/dist.js Show resolved Hide resolved
Copy link
Member

@danielrozenberg danielrozenberg left a comment

Choose a reason for hiding this comment

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

Oops, my mistake. Did that just now, looks good

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.

All comments addressed. Will merge after some local testing, and after Travis is happy.

build-system/tasks/dist.js Show resolved Hide resolved
build-system/tasks/build.js Show resolved Hide resolved
build-system/tasks/nailgun.js Show resolved Hide resolved
build-system/tasks/helpers.js Show resolved Hide resolved
Copy link
Collaborator

@estherkim estherkim left a comment

Choose a reason for hiding this comment

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

In before Travis

@rsimha
Copy link
Contributor Author

rsimha commented May 2, 2019

Locally tested the runtime (minified and not), and Travis is green. Merging.

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.

6 participants