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

Null safety #50

Closed
simolus3 opened this issue Nov 20, 2020 · 18 comments · Fixed by #51
Closed

Null safety #50

simolus3 opened this issue Nov 20, 2020 · 18 comments · Fixed by #51

Comments

@simolus3
Copy link
Contributor

Dart 1.12 is in beta and supports null safety - time to migrate packages. Can this package be migrated? I'm happy to open a PR for that.

@daegalus
Copy link
Owner

I have no problems moving to null safety, I just don't follow dart news closely and missed this. I can look into it this weekend, you are also welcome to make a PR if you get to it sooner.

Also need to read up on the changes and setup my versions appropriately

@simolus3
Copy link
Contributor Author

No worries - the crypto package doesn't have a migrated version on pub yet, so it might be a bit soon for this package to migrate. I'll get a PR ready with dependency overrides, but it might take some time until this can be published.

@mit-mit
Copy link

mit-mit commented Nov 26, 2020

package:crypto should have null safety enabled soon, see dart-lang/crypto#107

@grouma
Copy link

grouma commented Dec 7, 2020

Looks like this is mostly done. Is there a reason the null safe branch hasn't been merged into master? Can we merge and publish?

@simolus3
Copy link
Contributor Author

simolus3 commented Dec 7, 2020

I'll open another PR to remove the dependency overrides then. As soon as that's through I think this package is ready.

@simolus3
Copy link
Contributor Author

simolus3 commented Dec 7, 2020

Oh wait, it looks 2.2.0-nullsafety.0 of crypto is not yet on pub. We definitely need that to remove the current git dependency from this package.

@grouma
Copy link

grouma commented Dec 7, 2020

cc @natebosch is looking at publishing a null safe version of crypto. Tracking issue: dart-lang/crypto#105

@daegalus
Copy link
Owner

daegalus commented Dec 7, 2020

Ya, once crypto gets a nullsafety package on pub, I'll push out a nullsafety version of this. Just been waiting on dependencies.

@sma
Copy link

sma commented Dec 8, 2020

I accidentally migrated this package for myself because I didn't see the existing branch. I noticed that it is very easy to remove the convert dependency because that package is only used to do a trivial hex encoding. I propose to change the code like this to remove that dependency:

    // Easy number <-> hex conversion
    for (var i = 0; i < 256; i++) {
      var hex = i.toRadixString(16).padLeft(2, '0');
      _byteToHex[i] = hex;
      _hexToByte[hex] = i;
    }

@daegalus
Copy link
Owner

daegalus commented Dec 8, 2020

That is a good point. I think I can make the change to the nullsafety branch in a way that will keep nullsafety.

@daegalus
Copy link
Owner

Ok, removed dependency to convert, but the crypto dependency is pretty necessary, and until they release a nullsafety version, i cant publish to pub. (I tried it with dart-otp and it blocked the push). Until then it will stay on the nullsafety branch, but it is ready to merge to master as soon as that nullsafety crypto version is released.

@daegalus
Copy link
Owner

daegalus commented Dec 30, 2020

Ugh, still a no go. It seems because I use Crypto as a direct dependency, the Override applies to it also for some reason and won't let me publish

Package validation found the following error:
* Your pubspec.yaml must not override non-dev dependencies.
  This ensures you test your package against the same versions of its dependencies
  that users will have when they use it.

It is still related to test depending on web_socket_channel which has no nullsafety version and depends on the non-nullsafety version of crypto.

@mit-mit
Copy link

mit-mit commented Jan 5, 2021

Crypto has been published: https://pub.dev/packages/crypto/versions/3.0.0-nullsafety.0

@daegalus
Copy link
Owner

daegalus commented Jan 6, 2021

I am aware, but if you look at the last 2 references, and hte post before them. There are other issues that need to be solved before I can publish. I am currently blocked from publishing until those changes are made. I have those tickets open tracking it.

@simolus3
Copy link
Contributor Author

simolus3 commented Jan 6, 2021

FWIW I think we can do the following to publish an update now:

  1. Remove the test dependency and dependency_overrides
  2. Run pub get, ignore errors in test/
  3. pub lish
  4. Revert 1

To my best knowledge, that shouldn't break anything since our test dependency is a dev-dependency not passed to users. It might hurt pub scores, and it certainly feels fragile. It would also push the incompatibility to downstream users, since they probably depend on web_socket_channel, analyzer or build implementations that all use convert.

@daegalus
Copy link
Owner

daegalus commented Jan 6, 2021

I'll think about that. I feel passing the buck down to downstream users would just open me up to a whole bunch of new issues being opened about how it's breaking builds.

Also that does seem super fragile. Honestly I think dart devs should have finished porting all of their stuff before telling everyone else to do it. I see so many open issues about nullsafety versions and lots of blockers on dart team owned libraries not being ported.

I might still do your suggestion. Let me think on it.

@daegalus
Copy link
Owner

After getting the same instructions from a Dart dev. I have removed test/dev_dependencies long enough to push, and pushed up the change.

3.0.0-nullsafety.0 is published. I will now close this ticket. Please open a new one if there are any other problems

@grouma
Copy link

grouma commented Jan 21, 2021

Thank you!

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.

5 participants