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

Add support for nullsafety #2920

Closed
vanlooverenkoen opened this issue Nov 22, 2020 · 28 comments · Fixed by #2996
Closed

Add support for nullsafety #2920

vanlooverenkoen opened this issue Nov 22, 2020 · 28 comments · Fixed by #2996

Comments

@vanlooverenkoen
Copy link

I am upgrading my package (Kiwi) gbtb16/kiwi#51

But it seems that build & build_runner is not yet migrated to support nullsafety. Is there a timeline on when this could be added?

@jakemac53
Copy link
Contributor

We are blocked for a while on upstream dependencies, namely analyzer.

Once we are unblocked we will migrate, but there is no timeline right now.

@vanlooverenkoen
Copy link
Author

Alright thanks. :) I already created some tickets for some dart-lang packages. So we can track this.

@jakemac53
Copy link
Contributor

Ya, analyzer itself has a lot of deps as well, we have been working on migrating those though. But they have a large and very legacy codebase (originally ported from Java) so its going to be..... something to migrate it 🤣

@vanlooverenkoen
Copy link
Author

@jakemac53 do you know if ther already is a ticket for tracking the nullsafety support for analyzer? otherwise we can link these tickets together. If not I will create a new ticket

@jakemac53
Copy link
Contributor

jakemac53 commented Nov 23, 2020

It is being tracked somewhere, but I think not in a github issue (that team doesn't use them for tracking generally afaik). It definitely doesn't hurt to file one though so its easier to be notified when it is completed, and I can link it on a separate tracking sheet we have so hopefully it does get closed when the migration finishes.

@vanlooverenkoen
Copy link
Author

Alright prefect I will create another one

@vanlooverenkoen
Copy link
Author

Blocked because of: dart-lang/sdk#44290

@mnordine
Copy link

This is really too bad. I'm guessing there's a ton of packages/apps blocked on this, including our own, ☹️

@jakemac53
Copy link
Contributor

Note that this does not block builders from generating null safe code (these already exist actually).

It blocks the build script from running with sound null safety, and blocks code generators from migrating their generator code.

That is a long way of saying that this doesn't need to block applications which use generated code.

@mnordine
Copy link

mnordine commented Nov 23, 2020

We're using a library that uses build_runner and build_web_compilers, and we use them in our app as well (convert yaml to json). Where does that leave us? I'm getting analyzer static errors due to build_runner

@jakemac53
Copy link
Contributor

jakemac53 commented Nov 23, 2020

We're using a library that uses build_runner and build_web_compilers, and we use them in our app as well (convert yaml to json).

I don't understand fully the situation here - you generally wouldn't have imports of these libraries within your app. If you do, then that will be blocked until we can migrate.

Code generators should generally be split up into two libraries - one for the generator (only imported by the build script), and one for any runtime libs which should not import package:build, package:analyzer, etc.

The runtime libs can be migrated at any time in that case, and unblock migrating apps. The build time libs will be blocked on this issue.

@jakemac53
Copy link
Contributor

jakemac53 commented Nov 23, 2020

It might be good to also clarify that you can opt out individual libs using a language version comment before any declarations in your file:

// @dart=2.9

So that would need to be used in order to opt out the generator libs.

@mnordine
Copy link

On a phone, but I will clarify later today

@vanlooverenkoen
Copy link
Author

@jakemac53 yes indeed. For kiwi only the generator needs to be updated. So I will have to wait

@mnordine
Copy link

mnordine commented Nov 24, 2020

@jakemac53

Package in question is StageXL. I think it uses build_runner, build_web_compilers as dev deps to run the example in that repo.

https://github.com/bp74/StageXL/blob/non-nullable/pubspec.yaml#L21

As I mentioned, our app uses a build configuration that converts yaml files to json. So our build.yaml looks like:

targets:

  $default:
    sources:
      {"exclude" : ["bin/**"]}
    builders:
      build_web_compilers|dart_source_cleanup:
        release_options:
          enabled: false
      build_web_compilers|dart2js_archive_extractor:
        options:
          filter_outputs: true
      build_web_compilers|entrypoint:
        generate_for:
          - web/main.dart
        dev_options:
          compiler: dartdevc
        release_options:
          compiler: dart2js
          dart2js_args:
            # See https://webdev.dartlang.org/tools/dart2js#size-and-speed-options
            - --show-package-warnings
            - -O4
            - -Ddebug=false

builders:
  yaml2JsonBuilder:
    import: "package:main/builder.dart"
    builder_factories: ["yaml2JsonBuilder"]
    build_extensions: {".yaml": [".json"]}
    auto_apply: root_package
    defaults:
      generate_for:
        include:
          - web/assets/**.yaml

global_options:
  build_web_compilers:ddc:
    options:
      environment:
        debug: 'true'

And we have a builder like so:

import 'dart:convert';
import 'package:build/build.dart';
import 'package:yaml/yaml.dart';

Builder yaml2JsonBuilder(BuilderOptions options) => Yaml2JsonBuilder();

class Yaml2JsonBuilder implements Builder
{
  @override
  Future<void> build(BuildStep buildStep) async
  {
    final inputId = buildStep.inputId;
    final asset = inputId.changeExtension('.json');
    final yaml = await buildStep.readAsString(inputId);
    await buildStep.writeAsString(asset, _convert(yaml));
  }

  String _convert(String yaml)
  {
    const encoder = JsonEncoder();
    final map = loadYaml(yaml);
    return encoder.convert(map);
  }

  @override
  final buildExtensions = {'.yaml': ['.json']};
}

So we have dev deps on build_runner, build_web_compilers, like so:

dependencies:
  stagexl:
    git:
      url: https://github.com/bp74/StageXL.git
      ref: non-nullable

dev_dependencies:
  yaml: ^3.0.0-nullsafety
  meta: ^1.3.0-nullsafety.6

  build_runner: any
  build_web_compilers: any

But pub can't resolve without errors:

Because build_config >=0.3.1+1 depends on yaml ^2.1.11 and build_config >=0.1.1 <=0.3.1 requires SDK version >=1.22.1 <2.0.0 or >=2.0.0-dev.54 <2.0.0, build_config >=0.1.1 requires yaml ^2.1.11.
Thus, every version of build_web_compilers requires yaml ^2.1.11.
So, because main depends on both yaml ^3.0.0-nullsafety and build_web_compilers any, version solving failed.

@jakemac53
Copy link
Contributor

@mnordine this is a separate issue it looks like - you just need us to allow the latest yaml version so you can get a pub solve.

We will be working on that as well soon, it shouldn't actually be breaking but other deps of ours also have to be updated to allow it.

@daniel-v
Copy link

Out of curiosity: when the env considers if the entire dependency tree is made up of null-safe code, does it also consider dev_dependencies? If not, this not being ported to null-safe code is not that big of a deal.

@jakemac53
Copy link
Contributor

jakemac53 commented Dec 10, 2020

Depends on what you mean by "the env".

Tools that run/compile apps (flutter, dart vm, build_runner) only look at the entrypoint file, if that is opted in they attempt to run with sound null safety. Assuming that all transitive imports of that library are migrated, this will work fine. Otherwise it will fail and tell you to run with --no-sound-null-safety.

So that is a long winded way of saying yes this is not a big deal. We definitely want to unblock code generator authors from using null safety, but that is a much smaller subset of users than the people who use those code generators, and those people are not blocked (or I should say, the code generator authors are unblocked from generating null safe code for their users).

@JohnGalt1717
Copy link

The problem is that shelf is used for build_runner and a ton of other stuff and the dependency of that is http_parser which is used by http which almost every app will use.

Thus it's basically impossible in any real world project to use build_runner and do type safety as it stands right now.

@jakemac53
Copy link
Contributor

@JohnGalt1717 If I understand what you mean correctly, we can resolve that by expanding the constraints on our dependencies without migrating ourselves to resolve these issues.

There are likely a number of them that we can add support for now and we can work on that soon.

@JohnGalt1717
Copy link

@jakemac53 that would be excellent because I think build_runner will be a major blocker for a lot of projects to convert to null safety otherwise which will create a chicken and egg problem.

@jakemac53
Copy link
Contributor

@JohnGalt1717 I don't see a new version of shelf yet - we do have a bunch of other deps that I am upgrading (but not the ones you mention here for now). Let me know if I am missing something.

@JohnGalt1717
Copy link

So if you go to the http package repository you can see their dependancies in their master branch which is their null safety stuff (they haven't published yet)

I believe it isn't shelf but http_replay or something like that in shelf that is the issue now that I look carefully at why it's freaking out. I guess shelf needs to also be updated?

@jakemac53
Copy link
Contributor

We only depend directly on shelf, shelf_web_socket, and http_multi_server as far as http/server related deps that I can see - so once those are updated to support the new http releases (which will be blocked on http itself as well I think) then we can roll those.

We will try to keep relatively on top of this though and if you notice something has been released feel free to open an issue asking for support. I am working on some releases now to support things like async, collection, convert, and others.

@cyanglaz
Copy link

Looks like the in_app_purchase plugin depends build_runner. We are trying to migrate the plugin to null safety but we can't with the current dependency.
See: https://github.com/flutter/plugins/blob/master/packages/in_app_purchase/pubspec.yaml#L15

@simolus3
Copy link
Contributor

Note that your migration isn't actually blocked if you're only using build dependencies that you don't import (e.g. build_runner). Only the non-dev dependencies that you typically use in your Dart code are necessary.

This issue mainly tracks changes needed to allow builder authors to migrate their package, most builder users can migrate today.

@mateusfccp
Copy link

So, it seems that analyzer has finally been migrated to null-safety.

dart-lang/sdk#44290 (comment)

Is there any plans now to migrate build to null-safety?

@jakemac53
Copy link
Contributor

There is some work ongoing in #3003 to allow the latest analyzer as well as #2996 to start migrating some of our packages.

jakemac53 pushed a commit that referenced this issue Feb 19, 2021
Migrate packages typically imported by build authors to null-safety (part 1 /3). Closes #2920

In this PR, I migrate

- the `build` package
- the `build_test` package (or rather, the parts of that package that don't import `build_resolvers`)
- the `scratch_space` package

After the `graphs` package is migrated, I'll open another PR to migrate the `build_resolvers` package. When that one is done too, we can start to opt the tests of `build` and `build_test` into null-safety and publish, thus enabling build authors to write and test their builders with strong null-safety. 

Migrating the rest of the build system is not a goal for the near future, so we won't run builds with strong null-safety.
@simolus3 simolus3 mentioned this issue Mar 6, 2021
@jakemac53 jakemac53 mentioned this issue Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants