★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/22104Nouvelle recherche | Portail wanayoo
Skip to content

/node

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: add doc for --loader option #22104

Closed
wants to merge 6 commits into from

Conversation

Reviewers

@rubys rubys

@devsnek devsnek

@vsemozhetbyt vsemozhetbyt

@jdalton jdalton

@jasnell jasnell

@benjamingr benjamingr

@sagirk sagirk

Assignees
No one assigned
Labels
ES Modules cli doc
Projects
None yet
Milestone
No milestone
9 participants
@SirR4T @vsemozhetbyt @devsnek @jdalton @rubys @jasnell @benjamingr @sagirk @nodejs-github-bot
@SirR4T
Copy link
Contributor

SirR4T commented Aug 3, 2018

Fixes #21230

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added cli doc labels Aug 3, 2018

@jasnell

jasnell reviewed Aug 3, 2018
View changes

doc/api/cli.md Outdated
added: v9.0.0
-->

Specify a customer loader, to load [ECMAScript Modules][].

This comment has been minimized.

Sign in to view
Copy link
@jasnell

jasnell Aug 3, 2018

Member

s/customer/custom

Also, it should be noted that this is still an experimental feature.

This comment has been minimized.

Sign in to view
Copy link
@jdalton

jdalton Aug 4, 2018

Member

I believe it's been brought up before but given that --loader is still experimental I think it would it be better if it were --experimental-loader in much the same way other experimental stuff is prefixed with --experimental-

This comment has been minimized.

Sign in to view
Copy link
@benjamingr

benjamingr Aug 4, 2018

Member

@jdalton given you have to run node with --experimental-modules anyway to use this - why?

This comment has been minimized.

Sign in to view
Copy link
@SirR4T

SirR4T Aug 6, 2018

Author Contributor

thanks @jasnell ! fixed the typo.
Should i mention here, that it needs to be used with --experimental-modules?

This comment has been minimized.

Sign in to view
Copy link
@jdalton

jdalton Aug 6, 2018

Member

@benjamingr

given you have to run node with --experimental-modules anyway to use this - why?

I like the consistency of the --experimental- prefix for experimental things. The prefix makes it explicit that the feature/sub-feature is experimental.
@vsemozhetbyt

vsemozhetbyt reviewed Aug 3, 2018
View changes

doc/api/cli.md Outdated
@@ -687,3 +694,4 @@ greater than `4` (its current default value). For more information, see the
[debugger]: debugger.html
[emit_warning]: process.html#process_process_emitwarning_warning_type_code_ctor
[libuv threadpool documentation]: http://docs.libuv.org/en/latest/threadpool.html
[ECMAScript Modules]: esm.html#loader_hooks

This comment has been minimized.

Sign in to view
Copy link
@vsemozhetbyt

vsemozhetbyt Aug 3, 2018

Member

esm.html#esm_loader_hooks?

This comment has been minimized.

Sign in to view
Copy link
@SirR4T

SirR4T Aug 6, 2018

Author Contributor

thanks! fixed now.
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 3, 2018

cc @nodejs/modules-active-members?
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 3, 2018

It seems doc/node.1 and PrintHelp() in src/node.cc should also be updated?

@vsemozhetbyt vsemozhetbyt added the ES Modules label Aug 3, 2018

@SirR4T SirR4T force-pushed the SirR4T:addCliDocForLoader branch from 2406e35 to 9d6356d Aug 6, 2018

@SirR4T

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

SirR4T commented Aug 6, 2018

@vsemozhetbyt Added docs for node.1 and src/node.cc, though I couldn't maintain consistency in the doc strings. Is that fine?
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 6, 2018

cc @nodejs/documentation for node.1 and src/node.cc changes.
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 6, 2018

Linter issue:

doc/api/cli.md
  1:1  warning  Missing newline character at end of file  final-newline  remark-lint
@SirR4T

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

SirR4T commented Aug 6, 2018

@vsemozhetbyt thanks, installed markdown linter now. Will ensure i run this, before pushing any more doc updates.
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 6, 2018

cc @nodejs/build-infra re CI error:

not ok 2295 sequential/test-fs-watch
  ---
  duration_ms: 0.111
  severity: fail
  exitcode: 1
  stack: |-
    internal/fs/watchers.js:170
        throw error;
        ^
    
    Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.0/watch.txt'
@vsemozhetbyt

This comment was marked as outdated.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 7, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16256/
@benjamingr

benjamingr approved these changes Aug 7, 2018
View changes

@SirR4T SirR4T force-pushed the SirR4T:addCliDocForLoader branch from 0dea374 to 341d4b0 Aug 8, 2018

@vsemozhetbyt

This comment was marked as outdated.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 8, 2018

Another CI: https://ci.nodejs.org/job/node-test-pull-request/16275/
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 8, 2018

@nodejs/documentation Can anybody confident enough look into node.1 and node.cc changes so we could have some more LGTM?
@jasnell

jasnell approved these changes Aug 8, 2018
View changes

@vsemozhetbyt vsemozhetbyt referenced this pull request Aug 8, 2018

Closed

test: remove common.fileExists() #22151

3 of 3 tasks complete
@vsemozhetbyt

This comment was marked as resolved.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 8, 2018

Re CI fails: #22151 (comment)
@vsemozhetbyt

This comment was marked as outdated.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 8, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16281/

@SirR4T SirR4T force-pushed the SirR4T:addCliDocForLoader branch from 341d4b0 to a88503c Aug 9, 2018

@vsemozhetbyt

This comment was marked as outdated.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 9, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16292/
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 9, 2018

Another CI: https://ci.nodejs.org/job/node-test-pull-request/16313/

If CI is green, I will land this PR tomorrow if nobody objects.

@SirR4T SirR4T force-pushed the SirR4T:addCliDocForLoader branch from a88503c to b4d15f5 Aug 10, 2018

@sagirk

sagirk approved these changes Aug 10, 2018
View changes

Copy link
Member

sagirk left a comment

LGTM! 🎉
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 10, 2018

In last CIs, it seems we have some repetitive fails in parallel/test-cli-node-print-help in node-test-commit-linux-containered that may be relative. Can anybody look into?

One more CI to be sure: https://ci.nodejs.org/job/node-test-pull-request/16332/
@SirR4T

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

SirR4T commented Aug 10, 2018 •
edited

@vsemozhetbyt sorry, this maybe completely off topic, but why are --experimental-modules, --experimental-vm-modules, and --preserve-symlinks predicated on having i18n support?
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 10, 2018

Sorry, this also puzzled me and I thought that it was because I just did not know C++ :)
@vsemozhetbyt

vsemozhetbyt reviewed Aug 11, 2018
View changes

src/node.cc
@@ -2617,6 +2617,9 @@ static void PrintHelp() {
" --inspect[=[host:]port] activate inspector on host:port\n"
" (default: 127.0.0.1:9229)\n"
#endif // HAVE_INSPECTOR
" --loader=file (with --experimental-modules) use the \n"

This comment has been minimized.

Sign in to view
Copy link
@vsemozhetbyt

vsemozhetbyt Aug 11, 2018 •
edited

Member

If I understand @devsnek's comment correctly, this line causes the parallel/test-cli-node-print-help.js to fail when node is compiled without Intl. Maybe we can replace (with --experimental-modules) with something else, like(with experimental-modules flag)?

This comment has been minimized.

Sign in to view
Copy link
@vsemozhetbyt

vsemozhetbyt Aug 11, 2018

Member

However, if this flag can be used only with --experimental-modules, it seems we should place it inside the same if-clause and update the parallel/test-cli-node-print-help.js.

This comment has been minimized.

Sign in to view
Copy link
@devsnek

devsnek Aug 11, 2018

Member

i'm opening a pr to fix the flags. this line should stay how it is.
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 11, 2018

I will add "blocked" label till #22271 is landed. Please, correct me if I am wrong.

@vsemozhetbyt vsemozhetbyt added the blocked label Aug 11, 2018

@jdalton

jdalton requested changes Aug 11, 2018
View changes

Copy link
Member

jdalton left a comment

Please reword the main description of --loader to express "experimental" somewhere in it. Having it simply implied by association isn't great.

@SirR4T SirR4T force-pushed the SirR4T:addCliDocForLoader branch from b4d15f5 to b4897b6 Aug 16, 2018

@SirR4T

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

SirR4T commented Aug 16, 2018

@jdalton : updated the doc for --loader option. Sounds good? Open to other wordings / text as well, to put the "experimental" point across.
@jdalton

This comment has been minimized.

Sign in to view
Copy link
Member

jdalton commented Aug 16, 2018

@SirR4T It's better. But --loader is also experimental. So maybe something like

Specify the file of the custom [experimental ECMAScript module][] loader.

@SirR4T SirR4T force-pushed the SirR4T:addCliDocForLoader branch from b4897b6 to 188fb32 Aug 16, 2018

@SirR4T

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

SirR4T commented Aug 16, 2018

thanks, @jdalton , fixed.
@vsemozhetbyt

vsemozhetbyt reviewed Aug 16, 2018
View changes

doc/api/cli.md Outdated
@@ -687,3 +694,4 @@ greater than `4` (its current default value). For more information, see the
[debugger]: debugger.html
[emit_warning]: process.html#process_process_emitwarning_warning_type_code_ctor
[libuv threadpool documentation]: http://docs.libuv.org/en/latest/threadpool.html
[experimental ECMAScript Module]: esm.html#esm_loader_hooks

This comment has been minimized.

Sign in to view
Copy link
@vsemozhetbyt

vsemozhetbyt Aug 16, 2018 •
edited

Member

Nit: reference list is sorted in ASCII order, so this item needs to be placed before the [libuv threadpool documentation].

This comment has been minimized.

Sign in to view
Copy link
@SirR4T

SirR4T Aug 16, 2018 •
edited

Author Contributor

Sure, will fix that. Any way we could add this as a rule to make lint?

This comment has been minimized.

Sign in to view
Copy link
@vsemozhetbyt

vsemozhetbyt Aug 16, 2018

Member

It would be helpful to have it as a rule as we are already a bit messy in some docs in these sections,

This comment has been minimized.

Sign in to view
Copy link
@SirR4T

SirR4T Aug 17, 2018

Author Contributor

Would updating remark-preset-lint-node to also include remark-lint-alphabetize-lists work? I expect this would cause build failures, if remark-preset-lint-node was updated before fixes for that landed in master. How would the transition be handled?

This comment has been minimized.

Sign in to view
Copy link
@vsemozhetbyt

vsemozhetbyt Aug 17, 2018

Member

I am not sure that remark-lint-alphabetize-lists can fix this case as reference lists are not common markdown lists. Maybe @rubys can tell if there is a ready easy solution for this?

This comment has been minimized.

Sign in to view
Copy link
@rubys

rubys Aug 17, 2018

Member

Remark calls those lines definitions. Looking at the source to remark-lint-alphabetize-lists
, if list where changed to definition on line 18, the code would do what you want.
@jdalton

jdalton approved these changes Aug 16, 2018
View changes

@vsemozhetbyt vsemozhetbyt removed the blocked label Aug 16, 2018

@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 16, 2018

#22271 is merged, so let's unblock and run CI:
https://ci.nodejs.org/job/node-test-pull-request/16490/
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 16, 2018 •
edited

Rebuild failed job: https://ci.nodejs.org/job/node-test-commit-linux-containered/6326/
And again: https://ci.nodejs.org/job/node-test-commit-linux-containered/6335/

@joyeecheung joyeecheung referenced this pull request Aug 17, 2018

Closed

CI Failures: 20180817 #14

@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Aug 18, 2018

Landed in 9d6619e
Thank you!

@vsemozhetbyt vsemozhetbyt closed this Aug 18, 2018

vsemozhetbyt added a commit that referenced this pull request Aug 18, 2018

@SirR4T @vsemozhetbyt
doc: add doc for --loader option 
PR-URL: #22104
Fixes: #21230
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Loading status checks…
9d6619e

targos added a commit that referenced this pull request Aug 19, 2018

@SirR4T @targos
doc: add doc for --loader option 
PR-URL: #22104
Fixes: #21230
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
c76fb12

@SirR4T SirR4T deleted the SirR4T:addCliDocForLoader branch Aug 22, 2018

targos added a commit that referenced this pull request Sep 3, 2018

@SirR4T @targos
doc: add doc for --loader option 
PR-URL: #22104
Fixes: #21230
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
e644088

@targos targos referenced this pull request Sep 5, 2018

Merged

Release proposal: v10.10.0 #22716

This was referenced Sep 6, 2018

Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.