Skip to content

updates to move away from dart:js_util #148

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

Merged
merged 6 commits into from
Feb 13, 2025
Merged

updates to move away from dart:js_util #148

merged 6 commits into from
Feb 13, 2025

Conversation

devoncarew
Copy link
Member

  • updates to move away from dart:js_util
  • run npm update to get the latest versions of node packages

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@@ -17,6 +17,7 @@ extension type Process(JSObject obj) {
external JSObject get _env;

/// Read the environment variable [variable].
// TODO(devoncarew): move away from using dart:js_util `getProperty`.
Copy link
Member Author

Choose a reason for hiding this comment

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

@srujzs - quick interop question. If I have a JS object, how to I access named fields (like 'PWD' or 'PATH').

Context: https://nodejs.org/api/process.html#processenv

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, JSObjectUnsafeUtilExtension.getProperty :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Should the docs for https://api.dart.dev/stable/latest/dart-js_interop_unsafe/JSObjectUnsafeUtilExtension.html be updated? It says but takes and returns a Dart value but the sig indicates it takes a dart value and returns a JSAny.

Copy link

Choose a reason for hiding this comment

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

Yup, or just []. You could also add an extension type for _env and add external fields with the expected names.

Thanks for the catch! https://dart-review.googlesource.com/c/sdk/+/409480

lib/main.dart Outdated
'allowRedirects': true,
'maxRedirects': 3,
'allowRetries': true,
'maxRetries': 3,
}) as JSObject?,
}.toJSBox,
Copy link
Member Author

Choose a reason for hiding this comment

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

@srujzs - also, I don't see a .toJS on Dart Maps, but do see a toJSBox. Here, I'm trying to convert to a regular JSObject.

Copy link

Choose a reason for hiding this comment

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

I'd just use .jsify() like you were before - it's just an extension method now. This should translate the map into a JS object using the string keys as property names just like before. toJSBox creates an explicit box around the object so that you can pass it back to the Dart runtime later so it's not the right fit.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 thanks

@devoncarew devoncarew marked this pull request as ready for review February 12, 2025 18:43
@devoncarew devoncarew requested a review from athomas February 12, 2025 22:22
@devoncarew devoncarew merged commit 1faf8e6 into main Feb 13, 2025
27 checks passed
@devoncarew devoncarew deleted the js_interop_updates branch February 13, 2025 16:01
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