Skip to content

build: serve dev-app with bazel #16829

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

Merged
merged 1 commit into from
Aug 21, 2019
Merged

Conversation

devversion
Copy link
Member

@devversion devversion commented Aug 20, 2019

  • Serves the dev-app with Bazel using ts_devserver (windows
    compatible since
    bazelbuild/rules_typescript@4155687)
  • Serves dev-app with AOT mode 💯
  • ibazel can be used for watch-mode and live-reloading

Currently due to limitations within the ts_devserver rule, livereloading with ibazel is not
working. Manual reloading still works. Created bazel-contrib/rules_nodejs#1036 for tracking.

Additionally rebuilding seems at first glance slower as with the gulp setup. This is because the dev-app has the same problem as the material-examples and ideally it would have a bazel package file per module (modules are already available due to lazy loading)

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 20, 2019
@devversion devversion marked this pull request as ready for review August 20, 2019 19:16
@devversion devversion requested a review from jelbourn as a code owner August 20, 2019 19:16
@devversion devversion added pr: merge safe target: patch This PR is targeted for the next patch release labels Aug 20, 2019
* Serves the dev-app with Bazel using `ts_devserver` (windows
compatible since
bazelbuild/rules_typescript@4155687)
* Serves dev-app with AOT mode.
* ibazel can be used for watch-mode and live-reloading

Currently due to limitations within the `ts_devserver` rule,
livereloading with ibazel is not working. Manual reloading still works.
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just one other question that I could go either way on.

# Job that builds the dev-app with AOT. In order to speed up this job, the release output
# from the workspace storage will be attached to this job.
# -----------------------------------------------------------------------------------------
build_devapp_aot:
Copy link
Member

Choose a reason for hiding this comment

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

Should we still build the devapp as a test?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be built as part of the bazel build job.

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

"test": "bazel test //src/... --test_tag_filters=-e2e,-browser:firefox-local --build_tag_filters=-browser:firefox-local --build_tests_only",
"test-firefox": "bazel test //src/... --test_tag_filters=-e2e,-browser:chromium-local --build_tag_filters=-browser:chromium-local --build_tests_only",
"lint": "gulp lint && yarn -s bazel:format-lint",
"e2e": "bazel test //src/... --test_tag_filters=e2e",
"deploy": "gulp deploy:devapp",
"deploy": "echo 'Not supported yet. Tracked with COMP-230'",
Copy link
Member

Choose a reason for hiding this comment

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

I moved this to the queue since I want to keep this deploy functional; we'll likely want to do some amount of IE and mobile testing with the MDC components as we progress

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker blocked This issue is blocked by some external factor, such as a prerequisite PR and removed action: merge The PR is ready for merge by the caretaker labels Aug 20, 2019
@jelbourn
Copy link
Member

We're going to hold off on merging this until follow-up work to speed up the dev-app rebuild by making the targets more granular

@devversion
Copy link
Member Author

devversion commented Aug 21, 2019

@jelbourn This should be ready to go once #16837 is approved. Then both can be merged.

@devversion devversion removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Aug 21, 2019
@devversion
Copy link
Member Author

@jelbourn I'll re-add merge ready as the other PR is approved and ready. Feel free to remove if you think something is still missing.

@devversion devversion added the action: merge The PR is ready for merge by the caretaker label Aug 21, 2019
@jelbourn jelbourn merged commit 1162048 into angular:master Aug 21, 2019
andrewseguin pushed a commit that referenced this pull request Aug 26, 2019
* Serves the dev-app with Bazel using `ts_devserver` (windows
compatible since
bazelbuild/rules_typescript@4155687)
* Serves dev-app with AOT mode.
* ibazel can be used for watch-mode and live-reloading

Currently due to limitations within the `ts_devserver` rule,
livereloading with ibazel is not working. Manual reloading still works.

(cherry picked from commit 1162048)
@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 Sep 21, 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