Skip to content

test: fix ts compiler errors - #23130

Merged
clydin merged 2 commits into
angular:mainfrom
jbedard:err-compile-err
Jun 6, 2022
Merged

test: fix ts compiler errors#23130
clydin merged 2 commits into
angular:mainfrom
jbedard:err-compile-err

Conversation

@jbedard

@jbedard jbedard commented May 12, 2022

Copy link
Copy Markdown
Contributor

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

jbedard commented May 18, 2022

Copy link
Copy Markdown
Contributor Author

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
Comment thread tests/legacy-cli/e2e/utils/fs.ts Outdated
Comment thread tests/legacy-cli/e2e/tests/i18n/ivy-localize-basehref.ts Outdated
Comment thread tests/legacy-cli/e2e/tests/build/platform-server.ts Outdated
@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

jbedard commented Jun 3, 2022

Copy link
Copy Markdown
Contributor Author

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.

@clydin clydin left a comment

Copy link
Copy Markdown
Member

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

jbedard commented Jun 6, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown

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