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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃彈 Re-implement gulp serve as an in-process server #24325

Merged
merged 6 commits into from Sep 3, 2019
Merged

馃彈 Re-implement gulp serve as an in-process server #24325

merged 6 commits into from Sep 3, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Sep 1, 2019

PR Highlights:

Coming up:

  • Reorganize all dev server files into a separate directory
  • Do more with lazy-building of JS files

Partial fix for #24141
Related to #16643, #18635, #24138, #24152, #24199, #24214, #24258, #24244, #24287

@rsimha rsimha self-assigned this Sep 1, 2019
@rsimha rsimha changed the title 馃彈 Rewrite gulp serve as an in-process server using gulp-connect 馃彈 Re-implement gulp serve as an in-process server Sep 1, 2019
@rsimha
Copy link
Contributor Author

rsimha commented Sep 1, 2019

Travis checks are green, and I've tested this in a few different ways. Adding some folks for extra review to spot corner cases that might need follow up fixes.

  • @alanorozco The AMP_TESTING env var hack has been removed. Dev dashboard should remain unchanged.
  • @estherkim The new server code has been reused for unit and integration tests.
  • @calebcordry nodemon and its --inspect flag have been removed. You should now be able to debug this like any othernode process.
  • @sparhami Disabling of extension caching has been moved to app.js and exposed via --no_caching_extensions.
  • @erwinmombay @zhouyx You folks wrote parts of the original server implementation. Lemme know if you think I've missed something.

@rsimha rsimha requested a review from sparhami September 2, 2019 01:46
@rsimha rsimha requested a review from zhouyx September 3, 2019 02:32
Copy link
Contributor

@zhouyx zhouyx left a comment

Choose a reason for hiding this comment

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

Thank you @rsimha for improve the server : ) LGTM 馃憤

Also talked offline, we will need to further improve the app.js code as now the file is getting too big. And the order of the proxy could lead to issues.
For example here: the no-caching-extensions option. Because it's placed at line 843, when we fetched something like /dist/rtv/*/v0/*.js, it won't be applied because of the order.
Or, setHeader() that's placed after the proxy could change the header value.

Also agreed on all request related middleware should go to app.js, while server setting related middleware should go to serve.js.

@rsimha
Copy link
Contributor Author

rsimha commented Sep 3, 2019

Thanks for the detailed review and discussion, @zhouyx! I agree that we should reorganize app.js to make it more predictable and maintainable. Since it's out of scope for this PR, let's deal with it in a follow up. I've filed #24333 to track this work.

Merging this PR now.

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.

None yet

4 participants