Skip to content

module: createRequireFromPath missed docs#24763

Closed
doertedev wants to merge 1 commit into
nodejs:masterfrom
doertedev:master
Closed

module: createRequireFromPath missed docs#24763
doertedev wants to merge 1 commit into
nodejs:masterfrom
doertedev:master

Conversation

@doertedev

Copy link
Copy Markdown

Fixes: #23710

Adding the missing documentation

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Fixes: nodejs#23710

Adding the missing documentation
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. labels Nov 30, 2018
@Trott

Trott commented Dec 1, 2018

Copy link
Copy Markdown
Member

/ping @guybedford

Comment thread doc/api/modules.md
```js
const { createRequireFromPath } = require('module');
const requireUtil = createRequireFromPath('../src/utils');
const requireUtil = createRequireFromPath('../src/utils'); // can also be a file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Am I mistaken or won't the code fail if utils is a directory unless it's like this?:

Suggested change
const requireUtil = createRequireFromPath('../src/utils'); // can also be a file
const requireUtil = createRequireFromPath('../src/utils/.');

And if so, then the comment is misleading because it implies that passing a directory without /. at the end will work.

And explaining the /. requirement is probably more trouble than it's worth.

Probably best to update this to point to a file and not a directory at all.

Suggested change
const requireUtil = createRequireFromPath('../src/utils'); // can also be a file
const requireUtil = createRequireFromPath('../src/utils/index.js');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes we should likely go with this last suggestion please.

@Trott

Trott commented Dec 1, 2018

Copy link
Copy Markdown
Member

(Commit message subsystem should be doc rather than module. Someone can fix that on landing, but if you end up rebasing or otherwise amending the commit message, you can save someone some keystrokes by updating it.)

@Trott

Trott commented Dec 1, 2018

Copy link
Copy Markdown
Member

Hi, @doertedev! Welcome, and thanks for the PR!

I might be mistaken, but think the current code sample needs to be changed, and we probably don't want to try to document using directories instead of filenames until the PR that adds support for trailing-slash-without-a-.-after-it lands. /ping @guybedford @targos

@doertedev

Copy link
Copy Markdown
Author

Eeer sorry in this case I must have misinterpreted #23710 (comment) which stated that a PR for dirs is already in place ??!?

@creativecomposer

Copy link
Copy Markdown

@doertedev the PR for dirs is not yet in place, so the example must be updated little differently, more in line with @Trott comment above.

I went ahead and did that instead of your PR. Is that OK for you?

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with the suggestion from @Trott

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR! We just need to update the example as @Trott describes.

Comment thread doc/api/modules.md
```js
const { createRequireFromPath } = require('module');
const requireUtil = createRequireFromPath('../src/utils');
const requireUtil = createRequireFromPath('../src/utils'); // can also be a file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes we should likely go with this last suggestion please.

@gireeshpunathil

Copy link
Copy Markdown
Member

ping @doertedev - can address the review comments?

@HarshithaKP

Copy link
Copy Markdown
Member

I will pick this up.

@HarshithaKP

Copy link
Copy Markdown
Member

Looking further, I see that the required changes are actually realized through #23818 and hence there is nothing left to do in this PR

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell

jasnell commented Jul 3, 2020

Copy link
Copy Markdown
Member

Looks like this can be closed.

@jasnell jasnell closed this Jul 3, 2020
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. module Issues and PRs related to the module subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

module: createRequireFromPath documentation is incorrect

10 participants