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

doc: import() #17395

Closed
wants to merge 3 commits into from
Closed

doc: import() #17395

wants to merge 3 commits into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Nov 30, 2017

I don't know if this should wait until import() is out from behind its own flag, but the change will need to happen at some point so here it is.

Checklist
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 30, 2017
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Change LGTM although I'd like it even more with the little nit I left addressed.

doc/api/esm.md Outdated
@@ -33,15 +33,14 @@ node --experimental-modules my-app.mjs
### Supported

Only the CLI argument for the main entry point to the program can be an entry
point into an ESM graph. In the future `import()` can be used to create entry
points into ESM graphs at run time.
point into an ESM graph. `import()` can also be used to create entry points
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking nit: I know this was in the original text, but import is a statement and not a function so the () should be removed. Same on line 43.

Copy link
Member Author

@devsnek devsnek Dec 1, 2017

Choose a reason for hiding this comment

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

should I just say "dynamic import" then? the majority of the community refers to it as import()

Copy link
Member

@Trott Trott Dec 1, 2017

Choose a reason for hiding this comment

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

Maybe leave it alone then. (Hey, I said it was non-blocking!) I see that even the TC39 proposal uses import() and refers to it as "function-like". https://github.com/tc39/proposal-dynamic-import

doc/api/esm.md Outdated
point into an ESM graph. In the future `import()` can be used to create entry
points into ESM graphs at run time.
point into an ESM graph. Dynamic import can also be used to create entry points
into ESM graphs at run time.
Copy link
Member

Choose a reason for hiding this comment

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

Only with an additional CLI flag --harmony_dynamic_import.

Copy link
Member Author

@devsnek devsnek Dec 6, 2017

Choose a reason for hiding this comment

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

like i said in the first comment, this might need to wait until that flag goes away, but i can doc it as being available from there also

Copy link
Member

Choose a reason for hiding this comment

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

I'd go for documenting it as it is to get this in right now.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 12, 2017
@BridgeAR
Copy link
Member

addaleax pushed a commit to addaleax/node that referenced this pull request Dec 13, 2017
PR-URL: nodejs#17395
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@addaleax
Copy link
Member

Landed in 11ebaff

@addaleax addaleax closed this Dec 13, 2017
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017
@devsnek devsnek deleted the patch-4 branch December 13, 2017 19:26
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
PR-URL: #17395
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
@MylesBorins
Copy link
Contributor

This should be backported in #17823

@targos
Copy link
Member

targos commented Jan 17, 2018

This landed too soon on v9.x. See #18202.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants