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

Generate library declarations with @JS annotations #156

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

srujzs
Copy link
Contributor

@srujzs srujzs commented Feb 1, 2024

This is needed to work around dart-lang/sdk#54801. When that issue is fixed, these declarations can be removed.

This is needed to work around dart-lang/sdk#54801.
When that issue is fixed, these declarations can be removed.
@srujzs srujzs requested a review from sigmundch February 1, 2024 20:54
lib/src/dom/anchors.dart Outdated Show resolved Hide resolved
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, feel free to ignore my question, it's probably not worth while anyways.

@@ -4,6 +4,9 @@

// Generated from Web IDL definitions.

@JS()
library;
Copy link
Member

Choose a reason for hiding this comment

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

curious, is the library; piece needed? I have this recollection that the first annotation before any import is still associated with the library, but I may be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You might be right (at least it compiles and works in this case), TIL. However, code_builder explicitly adds the library: https://github.com/dart-lang/code_builder/blob/3307105e57a1cbcc08faff250800bf3b93fbfe15/lib/src/emitter.dart#L521. I'm guessing they also didn't know this was possible!

Copy link
Member

Choose a reason for hiding this comment

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

;-) Here comes an absolutely random drive-by comment:

Perhaps the ability to associate metadata with the library as such when it occurs in front of the first import could be retired? It does seem to create an unnecessary ambiguity if that same metadata could have been intended to apply specifically to the first import directive...

Copy link
Member

@parlough parlough Feb 2, 2024

Choose a reason for hiding this comment

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

I think that makes a lot of sense @eernstg!

To ease the transition we could add https://dart.dev/tools/linter-rules/library_annotations to the core lints.

https://dart.dev/tools/linter-rules/dangling_library_doc_comments is already in the core set too :)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! And thanks for the supporting arguments! ;-)

Copy link
Member

Choose a reason for hiding this comment

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

I can be convinced of such change :). At a high level, I'd lean towards what's more common. If it's more common to use library annotations than import annotations, then it's nice not to have the library; directive. I agree, though, that when you do need to put an annotation on the first import, it will feed strange that you need to add a library directive above it to make it work.

Copy link
Member

Choose a reason for hiding this comment

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

The thing that helps here is that dart-doc comments have the same behavior. That tends to make the annotation quicks less surprising.

Copy link
Member

Choose a reason for hiding this comment

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

Yes! We had a bunch of vagueness about this. it's a bit more verbose, but it's 100% clear to have the library directive to hang things on!

@srujzs srujzs requested a review from kevmoo February 2, 2024 00:34
@srujzs srujzs merged commit 05e4713 into dart-lang:main Feb 2, 2024
9 checks passed
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