Skip to content

Conversation

@devoncarew
Copy link
Contributor

@devoncarew devoncarew commented Feb 23, 2023

Convert the implementation of the action to Dart:

  • a mostly direct translation from the Bash implementation to Dart
  • kept the dart.yml CI the same, so we could verify correct behavior of the port
  • as part of the port, addressed an issue related to auto-detection of the platform architecture (fix Automatically choose the correct architecture #59)
  • also, added a new action output - dart-version
  • done as part of general updates to the setup action #62
  • updated the changelog w/ the relevant changes, and rev'd the action version to 1.5.0-dev
  • updated with our std CONTRIBUTING.md file from dart-lang/.github
  • added a DEVELOPING.md file, with some info about how the action is built and how to work on it

@devoncarew devoncarew marked this pull request as ready for review March 1, 2023 00:37
@devoncarew devoncarew changed the title re-write into Dart convert the implementation of the action to Dart Mar 1, 2023
@devoncarew
Copy link
Contributor Author

@joshualitt + @srujzs - can you review the js interop code (lib/node/*), the bootstrap script (lib/main.mjs), and anything else that looks interesting? Thanks!

Copy link

@joshualitt joshualitt left a comment

Choose a reason for hiding this comment

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

The JS interop code looks great. I don't think there is anything that needs to change in this CL. One thing for us to ponder as team, there is some overlap in the JS interop used in this package and package:web. It may make sense for us to publish a node interop package so we can share this work. Perhaps some of our users may also want to use such a package.

Anyway, food for thought. Regardless, that is far outside the scope of this CL.

@devoncarew
Copy link
Contributor Author

devoncarew commented Mar 1, 2023

Thanks! cc'ing @mit for fyi on the PR; @athomas - do you mind doing a general review of this PR? It's substantially the same as the previous one (and trys to keep minimal diffs from the current bash impl).

@devoncarew devoncarew requested a review from athomas March 1, 2023 16:54
Copy link

@srujzs srujzs left a comment

Choose a reason for hiding this comment

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

LGTM too!

Copy link
Member

@athomas athomas left a comment

Choose a reason for hiding this comment

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

LGTM, didn't review JS interop because people that know what they're doing already did :)

@@ -1,8 +1,13 @@
# Dependabot configuration file - enable regular dependency checks.
version: 2

Copy link
Member

Choose a reason for hiding this comment

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

Just curious, why no package-ecosystem: pub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into some issues when eval'ing it when it was in beta (dependabot/dependabot-core#4979), and was generally waiting for them to be addressed before using the functionality more widely.

Copy link
Contributor Author

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

Thanks! PR updated.

@@ -1,8 +1,13 @@
# Dependabot configuration file - enable regular dependency checks.
version: 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into some issues when eval'ing it when it was in beta (dependabot/dependabot-core#4979), and was generally waiting for them to be addressed before using the functionality more widely.

@devoncarew devoncarew merged commit 699d685 into main Mar 4, 2023
@devoncarew devoncarew deleted the dart branch March 4, 2023 17:57
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.

Automatically choose the correct architecture

5 participants