-
Notifications
You must be signed in to change notification settings - Fork 32
Generate library declarations with @JS annotations #156
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
Conversation
This is needed to work around dart-lang/sdk#54801. When that issue is fixed, these declarations can be removed.
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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! ;-)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
This is needed to work around dart-lang/sdk#54801. When that issue is fixed, these declarations can be removed.