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

package:web migration page #5524

Merged
merged 11 commits into from
Feb 15, 2024
Merged

package:web migration page #5524

merged 11 commits into from
Feb 15, 2024

Conversation

MaryaBelanger
Copy link
Contributor

@MaryaBelanger MaryaBelanger commented Feb 9, 2024

This is a very high level migration page that mostly points to JS interop pages and the web repo for more detail.

Part of #4578

@dart-github-bot
Copy link
Collaborator

dart-github-bot commented Feb 9, 2024

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

https://dart-dev--pr5524-package-web-65mslxp4.web.app

```dart
_clickSubscription = window.onClick.listen(callback); // Remove
window.onclick = callback.toJS; // Add
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This example is from a doc written before the helper methods became a thing. I'm guessing technically there's a helper to address this kind of thing now, so maybe a bad example?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is, although the helpers are on Element, but yeah, I think the example is probably not what we want. We can put a TODO here to look at some of the migrations and look for a better example.

Copy link
Contributor

@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.

Thanks! I have an initial pass with some comments.

src/interop/js-interop/package-web.md Outdated Show resolved Hide resolved
src/interop/js-interop/package-web.md Outdated Show resolved Hide resolved
src/interop/js-interop/package-web.md Outdated Show resolved Hide resolved
src/interop/js-interop/package-web.md Outdated Show resolved Hide resolved
src/interop/js-interop/package-web.md Outdated Show resolved Hide resolved
```dart
_clickSubscription = window.onClick.listen(callback); // Remove
window.onclick = callback.toJS; // Add
```
Copy link
Contributor

Choose a reason for hiding this comment

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

There is, although the helpers are on Element, but yeah, I think the example is probably not what we want. We can put a TODO here to look at some of the migrations and look for a better example.

src/interop/js-interop/package-web.md Outdated Show resolved Hide resolved
automatic binding of async callbacks in the current zone.

To migrate, you can still to use zones,
but you will have to [write them yourself][zones].
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO on me to come up with an example.

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

@srujzs I added a rough draft of the comparison section. I'll definitely go over it again to improve the wording, but just want to get it out so you could take a look. Mostly wondering if that's the kind of content you had in mind. Thanks!

Does the following:

- Adds sections about virtual dispatch, conditional imports, and zones
- Rewords sections and headers to be a bit more accurate wrt naming
- Adds more context on renames and type checks
- Fixes old conditional import code
@srujzs
Copy link
Contributor

srujzs commented Feb 15, 2024

Thanks for the edits Marya! I went through and added a few edits (the commit message should contain a summary) and additional sections. From my perspective, this is good to go.

cc @sigmundch for a review. I'll tackle any comments in another PR instead so this can be unblocked.

@srujzs srujzs mentioned this pull request Feb 15, 2024
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.

The article looks really great and reads very nicely! Minor suggestions for latter, below.

src/content/interop/js-interop/package-web.md Outdated Show resolved Hide resolved
src/content/interop/js-interop/package-web.md Outdated Show resolved Hide resolved
src/content/interop/js-interop/package-web.md Outdated Show resolved Hide resolved
src/content/interop/js-interop/package-web.md Outdated Show resolved Hide resolved
src/content/interop/js-interop/package-web.md Outdated Show resolved Hide resolved
@MaryaBelanger MaryaBelanger merged commit 3f52a4b into js-interop Feb 15, 2024
2 checks passed
@kevmoo kevmoo deleted the package-web branch August 20, 2024 22:43
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.

5 participants