-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Use importjs
#17422
Use importjs
#17422
Conversation
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.
Nice! LGTM
@@ -70,7 +70,7 @@ type | |||
future*: T | |||
## Wraps the return type of an asynchronous procedure. | |||
|
|||
PromiseJs* {.importcpp: "Promise".} = ref object | |||
PromiseJs* {.importjs: "Promise".} = ref object |
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.
see also timotheecour#662
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.
LGTM. the reason this now works is that docs for nim js modules are now built with the new -b:js
flag
it should be discussed in nim-lang/RFCs#315, please add a note there if this use case isn't covered already |
This PR changes uses of
importcpp
toimportjs
where it doesn't make any problems.I looked at all files with "js" in the path name, those that I didn't change are:
js/dom.nim
js/dom_extensions.nim
js/jsconsole.nim
js/jscore.nim
system/jssys.nim
Using
importjs
there gives`importjs` for routines requires a pattern
errors, since apparentlyimportjs
doesn't behave the same asimportc
andimportcpp
. Should I open an issue for this or just add patterns for the imports (in a future PR)?