Skip to content

Conversation

@mertcanaltin
Copy link
Member

// TODO(BridgeAR): Add the requireStack as well.

I also added the require stack @RaisinTen

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 9, 2023
@mertcanaltin mertcanaltin requested a review from anonrig February 9, 2023 18:34
@mertcanaltin mertcanaltin requested a review from anonrig February 9, 2023 18:44
anonrig
anonrig previously approved these changes Feb 9, 2023
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

RSLGTM👍

@anonrig
Copy link
Member

anonrig commented Feb 9, 2023

cc @BridgeAR @RaisinTen

@anonrig anonrig dismissed their stale review February 13, 2023 12:23

Not ready

@mertcanaltin mertcanaltin requested review from anonrig and removed request for RaisinTen February 17, 2023 07:24
err.path = path.resolve(requestPath, 'package.json');
err.requestPath = originalPath;
// TODO(BridgeAR): Add the requireStack as well.

Copy link
Member

Choose a reason for hiding this comment

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

The requireStack must be added to this error. That does not seem to be addressed here?

'Please either fix that or report it to the module author',
'DeprecationWarning',
'DEP0128',
{ requireStack },
Copy link
Member

Choose a reason for hiding this comment

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

I am not certain what the intent here is. The warning seems independent from the TODO entry above?

@mertcanaltin
Copy link
Member Author

mertcanaltin commented Feb 27, 2023

Like Module._findPath and we think that changing the args list of this function will probably break some userspace packages. Do you have any comments on this? ❤️ @BridgeAR @anonrig

🎸 update : In the code block below, when the tryExtensions() function returns false, in the part where tryExtensions() is tried again with another parameter combination, I created a require stack when necessary

@mertcanaltin mertcanaltin reopened this Mar 13, 2023
@mertcanaltin mertcanaltin changed the title test: added the requireStack to loader.js [WİP]test: added the requireStack to loader.js Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants