Skip to content

test: fix ts compiler errors #23130

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 2 commits into from
Jun 6, 2022
Merged

test: fix ts compiler errors #23130

merged 2 commits into from
Jun 6, 2022

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented May 12, 2022

When compiling with the standard tsc (directly or via ts_library in bazel) there are a lot of compilation errors in the e2e tests which the runtime lib/bootstrap-local.js compilation seems to ignore. These are fixes required to get a standard compiler working such as bazel in #23074

@jbedard jbedard force-pushed the err-compile-err branch from 8d4e9bf to 1380be6 Compare May 12, 2022 19:49
@jbedard jbedard changed the title test: fix ts compiler error test: fix ts compiler errors May 12, 2022
@jbedard jbedard marked this pull request as ready for review May 12, 2022 20:20
@jbedard jbedard requested review from alan-agius4 and clydin May 12, 2022 20:20
@clydin clydin added the target: minor This PR is targeted for the next minor release label May 13, 2022
@jbedard jbedard force-pushed the err-compile-err branch 5 times, most recently from fb546ee to cf2f917 Compare May 17, 2022 17:50
@clydin clydin added the action: review The PR is still awaiting reviews from at least one requested reviewer label May 18, 2022
@jbedard jbedard force-pushed the err-compile-err branch 3 times, most recently from 31c6698 to 8523d25 Compare May 18, 2022 18:04
@jbedard
Copy link
Contributor Author

jbedard commented May 18, 2022

I've added a commit adding the BUILDs for the legacy-cli where all the compiler errors are and added the compilation of these to circleci. With all the changes landing keeping this PR up to date with the fixes without the a bazel build (#23074) is quite the hassle. Adding the actual bazel build to CI ensures this PR is up to date and correctly fixes the compilation errors.

While the bazel build of legacy-cli is unnecessary right now (other then for type checking) I think it's worth avoiding compilation errors. When these tests are moved to bazel (#23074) this will be required anyway, which hopefully will be finished soon 🤞

@jbedard jbedard force-pushed the err-compile-err branch 3 times, most recently from 52db560 to 9b79605 Compare May 24, 2022 18:12
@jbedard jbedard force-pushed the err-compile-err branch from 9b79605 to f6bdbd0 Compare May 26, 2022 19:54
@jbedard jbedard force-pushed the err-compile-err branch 5 times, most recently from 8c22b57 to f8b7931 Compare June 2, 2022 20:36
@jbedard jbedard force-pushed the err-compile-err branch from f8b7931 to 6bc9d97 Compare June 3, 2022 20:00
@jbedard jbedard requested a review from clydin June 3, 2022 20:01
@jbedard
Copy link
Contributor Author

jbedard commented Jun 3, 2022

I've reverted the addition of webdriver-manager to the package.json since #23245 switches to the one within protractor anyway (node_modules/protractor/bin/webdriver-manager). Since it's only used in a require.resolve it's a runtime dependency anyway and isn't required to compile.

Copy link
Member

@clydin clydin left a comment

Choose a reason for hiding this comment

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

LGTM.
Some of these changes would fail linting checks but these files are not linted yet and would need other cleanup prior to that anyway.

@clydin clydin removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 6, 2022
@jbedard
Copy link
Contributor Author

jbedard commented Jun 6, 2022

I can maybe do linting next but really need type checking before anything else 👍

@clydin clydin removed the request for review from alan-agius4 June 6, 2022 19:37
@clydin clydin added the action: merge The PR is ready for merge by the caretaker label Jun 6, 2022
@clydin clydin merged commit 9efa381 into angular:main Jun 6, 2022
@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 Jul 7, 2022
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 target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants