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

convert the implementation of the action to Dart #77

Merged
merged 11 commits into from Mar 4, 2023
Merged

convert the implementation of the action to Dart #77

merged 11 commits into from Mar 4, 2023

Conversation

devoncarew
Copy link
Member

@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
Member 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
Member 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!

lib/node/actions/http_client.dart Show resolved Hide resolved
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 :)

lib/main.dart Show resolved Hide resolved
lib/main.dart Outdated Show resolved Hide resolved
lib/main.dart Outdated Show resolved Hide resolved
lib/main.dart Outdated Show resolved Hide resolved
@@ -2,7 +2,12 @@
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
Member 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
Member 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.

@@ -2,7 +2,12 @@
version: 2

Copy link
Member 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.

lib/main.dart Outdated Show resolved Hide resolved
lib/main.dart Outdated Show resolved Hide resolved
lib/main.dart Outdated Show resolved Hide resolved
lib/main.dart Show resolved Hide resolved
@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