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

Support running node_interop in null safety mode #90

Merged
merged 3 commits into from
Mar 17, 2021
Merged

Support running node_interop in null safety mode #90

merged 3 commits into from
Mar 17, 2021

Conversation

Awjin
Copy link
Collaborator

@Awjin Awjin commented Dec 8, 2020

This allows downstream users to start migrating their packages to null safety.

Copy link
Owner

@pulyaevskiy pulyaevskiy left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, just a minor comment in pubspec. Please take a look.

node_interop/pubspec.yaml Outdated Show resolved Hide resolved
@Awjin
Copy link
Collaborator Author

Awjin commented Dec 10, 2020

Looks like this is breaking the e2e test that invokes build_runner (which does not have a null-safe version available).

@pulyaevskiy
Copy link
Owner

Looks like this is breaking the e2e test that invokes build_runner (which does not have a null-safe version available).

@Awjin

Build on stable fails because it runs on Dart 2.10.x and we've updated SDK constraint here.
Build on dev fails for multiple reasons. There seems to be an issue with build_node_compilers as well as in node_interop. I suspect e2e test fails because of the first one.

The node_interop one seems to be easy to fix, it's just a lint:
https://travis-ci.org/github/pulyaevskiy/node-interop/jobs/748668797#L557

We may need to look at fixing build_node_compilers first before landing this change...

@jathak
Copy link
Collaborator

jathak commented Dec 11, 2020

build_node_compilers probably can't be migrated to null-safety until build (and other related packages) are, and it's blocked on analyzer being migrated, which is apparently not happening until March at the earliest.

This shouldn't block migrating packages that only use build_node_compilers as a dev dependency though. Looking into this now...

@jathak
Copy link
Collaborator

jathak commented Mar 3, 2021

Dart 2.12 was released as stable this morning, so I'm going to check again to see if we can migrate without waiting on build. If not, it hopefully won't be a blocker for long, since analyzer's now been migrated and it looks like the build packages are being actively worked on.

@Awjin Awjin requested a review from pulyaevskiy March 11, 2021 01:09
@Awjin
Copy link
Collaborator Author

Awjin commented Mar 11, 2021

Ok, I've worked around all of the breakages. build_node_compilers works now—a deeper fix requires migrating it to nnbd, which is blocked on build migrating first.

@pulyaevskiy WDYT about releasing this first, then migrating build_node_compilers once it's possible to do so? This would unblock Dart Sass's migration to nnbd.

Copy link
Owner

@pulyaevskiy pulyaevskiy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@@ -1,11 +1,14 @@
targets:
$default:
sources:
- $package$
Copy link
Owner

Choose a reason for hiding this comment

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

Just сurious - is this a new addition in build package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is a new placeholder added in build_runner

@pulyaevskiy
Copy link
Owner

@Awjin sorry for the delay here.

WDYT about releasing this first, then migrating build_node_compilers once it's possible to do so? This would unblock Dart Sass's migration to nnbd.

This makes sense to me. Will merge this and publish a new version.

Thanks for working on this!

@pulyaevskiy pulyaevskiy merged commit b6a99bb into pulyaevskiy:master Mar 17, 2021
@stof stof mentioned this pull request Mar 17, 2021
@Awjin Awjin deleted the null-safe-interop branch March 17, 2021 17:23
@Awjin
Copy link
Collaborator Author

Awjin commented Mar 23, 2021

@pulyaevskiy Thanks for the review and merge! Would you be able to release a new version as well?

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 this pull request may close these issues.

3 participants