search

angulardart / angular

Fast and productive web framework provided by Dart
https://pub.dev/packages/angular
MIT License
1.83k stars 233 forks source link

Set language version explicitly #1936

Closed lukaszkolodziejczyk closed 3 years ago

lukaszkolodziejczyk commented 3 years ago

Normally, the lower bound of SDK constraint in pubspec.yaml determines whether the code should be opted-in for null-safety, or not. In case of Angular packages, nothing should be opted-in (e.g. for _angularast, we have sdk: ">=2.8.4 <3.0.0" which has lower bound lower than 2.10.0).

This seems to be respected during building (pub run build_runner build), because there were no errors in the pipeline execution. It does not seems to be respected during tests (pub run build_runner test), because there were null-safety related errors in that phase.

I did run tests without build_runner harness (so just pub run test) and surprisingly the tests passed (well, I had one failure in the compiler, but I think it was related to build_runner not being the tests executor). So apparently, build_runner uses some different SDK constraint when invoking package:test. Maybe it fixes it to the current SDK, which in my case would be sdk: "2.12.0-33.0.dev"?

Anyway, this verbose solution makes the the build_runner to correctly run tests in legacy mode.

The more long standing solution would be to make changes in build_runner directly, but I'm not quite sure if there are some design choices that we need to respect here, what do you think @kevmoo ?

kevmoo commented 3 years ago

@jakemac53 @natebosch – thoughts on help here? We shouldn't have to set these explicitly, right?

lukaszkolodziejczyk commented 3 years ago

An example for the previously mentioned pub run build_runner test vs pub run test issue for _angularast: -> pub run build_runner test output -> pub run test output

jakemac53 commented 3 years ago

What you are seeing I think is that the package config hasn't been updated to reflect the precompiled directory, so if your vm tests are not precompiled, package:test tries to compile them but it ends up opting in the world by default because none of those files have a default language version.

I thought that we had fixed this, but I could be wrong and I can try to confirm later today.

jakemac53 commented 3 years ago

I can indeed still repro this, and filed https://github.com/dart-lang/build/issues/2905.

You can work around it as you did here, or you could run vm tests using regular pub run test and only run web tests via pub run build_runner test, until I can get this fixed. It shouldn't take a long time to fix though.

jakemac53 commented 3 years ago

I have what should be a fix pending here https://github.com/dart-lang/build/pull/2907

jakemac53 commented 3 years ago

This should be released now, try upgrading (make sure you have the latest build_runner_core specifically) and running again?

lukaszkolodziejczyk commented 3 years ago

Yes, that works now, thank you Jake! I'm closing this PR now.