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

build: create custom bazel dev-server rule #16937

Merged

Conversation

devversion
Copy link
Member

@devversion devversion commented Sep 1, 2019

Implements a custom bazel dev-server rule that can be exposed eventually. The reason
we need a custom dev-server implementation is that the "ts_devserver" is not flexible
and needs to be synced into google3 (causing slow PR turnaround; and hestitancy to adding
new features. always the question of scope: for example)).

We need our own implemenation because we want:

Note: we can talk about exposing this to the public, but that's something we can do afterwards too. Also once this lands, we can use it for the e2e-app as well.

This also makes #16935 unnecessary as the root-cause is fixed.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 1, 2019
@devversion devversion force-pushed the build/implement-custom-devserver branch 2 times, most recently from 216c660 to 689ace1 Compare September 1, 2019 12:26
@devversion
Copy link
Member Author

Test failures are caused by a merge conflict in master that will be solved in #16928.

@devversion devversion marked this pull request as ready for review September 1, 2019 12:30
@devversion devversion requested review from jelbourn and a team as code owners September 1, 2019 12:30
@devversion devversion added pr: merge safe target: patch This PR is targeted for the next patch release labels Sep 1, 2019
tools/dev-server/index.bzl Outdated Show resolved Hide resolved
tools/dev-server/index.bzl Show resolved Hide resolved
tools/dev-server/launcher_template.sh Outdated Show resolved Hide resolved
tools/dev-server/index.bzl Outdated Show resolved Hide resolved
tools/dev-server/index.bzl Show resolved Hide resolved
tools/dev-server/index.bzl Show resolved Hide resolved
tools/dev-server/index.bzl Show resolved Hide resolved
tools/dev-server/launcher_template.sh Outdated Show resolved Hide resolved
@devversion devversion force-pushed the build/implement-custom-devserver branch from 689ace1 to 313346c Compare September 3, 2019 20:12
Implements a custom bazel dev-server rule that can be exposed eventually. The reason
we need a custom dev-server implementation is that the "ts_devserver" is not flexible
and needs to be synced into google3 (causing slow syncing; and hestitancy to adding
new features. always the question of scope).

We need our own implemenation because we want:

* Live-reloading to work (bazel-contrib/rules_nodejs#1036)
* HTML History API support (currently the ts_devserver always sends a 404 status code)
* Better host binding of the server (so that we can access the server on other devices)
* Flexibility & control (being able to do changes so that the dev-server fits our needs)
@devversion devversion force-pushed the build/implement-custom-devserver branch from 313346c to 00ce81b Compare September 3, 2019 20:14
@devversion
Copy link
Member Author

@jelbourn @josephperrott Addressed feedback. Please have another look.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Sep 3, 2019
@ngbot
Copy link

ngbot bot commented Sep 3, 2019

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure status "ci/circleci: tests_browserstack" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@jelbourn jelbourn merged commit 1c74518 into angular:master Sep 3, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants