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

[2.19] Add next-generation preview section to js-interop page #4528

Merged
merged 13 commits into from
Jan 27, 2023

Conversation

MaryaBelanger
Copy link
Contributor

@MaryaBelanger MaryaBelanger commented Jan 18, 2023

@parlough parlough requested review from parlough and srujzs January 18, 2023 21:59
@parlough parlough added the review.tech Awaiting Technical Review label Jan 18, 2023
@MaryaBelanger MaryaBelanger linked an issue Jan 18, 2023 that may be closed by this pull request
@srujzs
Copy link
Contributor

srujzs commented Jan 18, 2023

Thanks for adding this! I think it's what I had in mind for a future-proof interop, but I had a couple of comments.

As for landing this, I can go either way. @kevmoo, do you think the users you had in mind who want to use future-proof interop will find it valuable to have this here versus pointing them to the issue?

Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I have some minor suggestions, comments, and questions.

I definitely do think there is value in landing this since if the more future-proof "way" is already supported, this helps increase that awareness. In the best case this helps new JS-interop developers start by building this way rather than other ways while also helping existing JS-interop developers prepare or just know to keep an eye on this area.

This page is also linked to from the dart:js API docs as a destination to learn more about how developers should do things now. Sidenote: we should consider making the notice on that page more prominent.

src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
@MaryaBelanger
Copy link
Contributor Author

MaryaBelanger commented Jan 19, 2023

Thanks @parlough , I'll incorporate the rest of your suggestions (the ones I'll have to update manually) once @srujzs responds with the details.

In the best case this helps new JS-interop developers start by building this way rather than other ways

This is a good point and I think it changes the nature of the note saying this is only for people eager to prepare. I'll change the wording slightly to emphasize people who are just starting to implement js interop should do it this way.

Edit: created dart-lang/sdk#51075 to track this point:

This page is also linked to from the dart:js API docs as a destination to learn more about how developers should do things now. Sidenote: we should consider making the notice on that page more prominent.

src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
@parlough parlough changed the title [2.19] Future proof js interop section [2.19] Add next-generation preview section to js-interop page Jan 23, 2023
@MaryaBelanger
Copy link
Contributor Author

Wooo... ok, that was a big change, thanks for the input everyone.

I don't really expect anyone to review this right now, but it would be really nice to roll this in with the v2.19 branch, which is targeted for around 10am PST tomorrow.

@sigmundch if you could verify the extra context I added based on your comments, that would be so helpful! @mit-mit @parlough , your feedback or approval is greatly appreciated too if you can squeeze that in! Thank you

@parlough parlough requested a review from sigmundch January 24, 2023 03:44
@parlough parlough changed the base branch from v2.19 to main January 24, 2023 16:37
@parlough parlough changed the base branch from main to v2.19 January 24, 2023 16:40
@parlough parlough changed the base branch from v2.19 to main January 24, 2023 16:49
@github-actions
Copy link

github-actions bot commented Jan 24, 2023

Visit the preview URL for this PR (updated for commit 79ea217):

https://dart-dev--pr4528-future-js-interop-zfqketfc.web.app

(expires Fri, 03 Feb 2023 22:17:21 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: d851bc446d3c4d7394c5406c6f07255afc7075f3

Copy link
Member

@sigmundch sigmundch left a comment

Choose a reason for hiding this comment

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

Thanks Marya!

I know you are short on time, so just a few comments below. Will further changes be possible after the 10 AM time, or is the site going to be frozen for a while?

src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
@MaryaBelanger
Copy link
Contributor Author

@sigmundch Actually not short on time anymore! We re-targeted this to main so the rest of the 2.19 updates are not waiting on it (they're already in)

Will further changes be possible after the 10 AM time, or is the site going to be frozen for a while?

We can keep making changes, no deadline, and as far as I'm aware we don't freeze dart.dev ever anyway. So take your time! Thanks for your concern :)

Copy link
Member

@sigmundch sigmundch left a comment

Choose a reason for hiding this comment

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

Thank you so much @MaryaBelanger!

A few small suggestions below.

src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
@MaryaBelanger MaryaBelanger added review.copy Awaiting Copy Review and removed review.tech Awaiting Technical Review labels Jan 25, 2023
@MaryaBelanger MaryaBelanger requested review from atsansone and removed request for srujzs January 25, 2023 20:19
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
Copy link
Contributor

@atsansone atsansone left a comment

Choose a reason for hiding this comment

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

@MaryaBelanger : Gave a light review. There's a spelling mistake in the copy and a few suggestions.

Copy link
Member

@domesticmouse domesticmouse left a comment

Choose a reason for hiding this comment

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

Excited for these changes!

src/web/js-interop.md Outdated Show resolved Hide resolved
Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Really great job on this and adjusting for everyone's comments. Sorry I didn't respond earlier, but somehow I accidentally(?) unsubscribed to this PR (oops) and didn't realize Sigmund reviewed it again already. I have a few minor suggestions.

Also consider trying to remove toc: false from the frontmatter(top) of this page now as well and seeing how that looks, since there is more content on the page now. When there were no headings the TOC wasn't useful so it was added to remove it.

I've pushed some minor formatting fixes as well. The main thing was that in a list we indent the content of a list entry to match the text on its first line (https://github.com/dart-lang/site-shared/blob/main/doc/markdown.md#do-properly-indent-markdown-lists).

src/web/js-interop.md Outdated Show resolved Hide resolved
will help make interop even more idiomatic.

You can implement static interop using
the `package:js` annotation `@staticInterop`.
Copy link
Member

Choose a reason for hiding this comment

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

Optional: Consider linking this first @staticInterop to its API docs.

https://pub.dev/documentation/js/latest/js/staticInterop-constant.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk why I can't make that link work with {{site.pub-api}}/ syntax. I've tried a few ways but get a 404 every time

{{site.pub-api}}/staticInterop-constant
{{site.pub-api}}/js/staticInterop-constant
{{site.pub-api}}/js/js/staticInterop-constant
{{site.pub-api}}/js/latest/js/staticInterop-constant

Copy link
Member

Choose a reason for hiding this comment

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

@MaryaBelanger Currently pub.dev and api.dart.dev aren't set up to handle a missing .html, I believe the last one should work:

{{site.pub-api}}/js/latest/js/staticInterop-constant.html

src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
src/web/js-interop.md Outdated Show resolved Hide resolved
@srujzs
Copy link
Contributor

srujzs commented Jan 26, 2023

I can't tell if my comments have been accidentally duplicated or something - GitHub UI is confusing to me.

Anyways, they've all been addressed so you can ignore them. This looks great, thanks for working on this!

@parlough
Copy link
Member

parlough commented Jan 26, 2023

@srujzs Looks like the comments just weren't published until now for some reason, but I can see them now :)

We appreciate all of your insights, comments, and answers! :D

@srujzs
Copy link
Contributor

srujzs commented Jan 26, 2023

@srujzs Looks like the comments just weren't published until now for some reason, but I can see them now :)

!!! Ugh, sorry about that, I guess I needed to click the "Start Review" button to publish. Luckily, Siggi's comments encapsulated all of mine.

@MaryaBelanger
Copy link
Contributor Author

Added a couple minor edits just now. I'll wait a little while to merge just in case any final concerns are lingering, but I think this is done (@srujzs your comment conundrum confused me a little too 😅 I think everything you talked about is incorporated? But let me know if there's anything specific you want to reword/remove/link to/etc.)

Thanks everyone!

@MaryaBelanger MaryaBelanger merged commit 69b3e85 into main Jan 27, 2023
@parlough parlough deleted the future-js-interop branch January 27, 2023 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review.copy Awaiting Copy Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[2.19] Document future-proof JS-interop
8 participants