FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

WIP: module,errors: port module to internal/errors by rauno56 · Pull Request #11565 · nodejs/node · GitHub

/ node Public

WIP: module,errors: port module to internal/errors - #11565

Closed
rauno56 wants to merge 3 commits into
nodejs:masterfrom
rauno56:module-to-internal-errors
ConversationCommits (3)ChecksFiles changed
Closed

WIP: module,errors: port module to internal/errors#11565
rauno56 wants to merge 3 commits into
nodejs:masterfrom
rauno56:module-to-internal-errors

Conversation

rauno56 commented Feb 26, 2017 •
edited
Loading

Copy link
Copy Markdown

Porting module.js to use internal/errors.js according to tracking issue #11273 and sample PR #11294.

Porting module can be tricky because it's errors are commonly parsed in user-land.

This code sets some properties on caught errors from JSON parsing(example: https://github.com/nodejs/node/blob/master/lib/module.js#L94-L95) should those be left unchanged(as those are really not node libs' errors) or rethrown as new internal error(as we want every error from node to become standardized)?

Additionally to me it makes sense to also leave in setting error.code for backwards compatibility on MODULE_NOT_FOUND error(https://github.com/nodejs/node/blob/master/lib/module.js#L471). Case against that would again be that if we're going to break backwards compatibility, let's just clean it up really nice as we're doing that.

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
Affected core subsystem(s)

errors, module

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. module Issues and PRs related to the module subsystem. labels Feb 26, 2017
rauno56 mentioned this pull request Feb 26, 2017
Closed
80 tasks
rauno56 changed the title WIP: module,errors: port module not found error to internal/errors WIP: module,errors: port module to internal/errors Feb 26, 2017

ChALkeR commented Feb 26, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

This probably contradicts with modules being documented as Locked.
Refs: #6528, #11200.

rauno56 commented Feb 27, 2017

Copy link
Copy Markdown
Author

Got an answer to MODULE_NOT_FOUND concern.
Adding a error code behind a Symbol key doesn't contradict with the Locked status as far as I understand?

rauno56 commented Feb 27, 2017

Copy link
Copy Markdown
Author

Timers are also Locked, but the fact is not mentioned in the PR touching that.
Refs: #11384

ChALkeR commented Feb 28, 2017

Copy link
Copy Markdown
Member

@rauno56 It was mentioned in #11580 that was linked to #11384, but I added an explicit mention there, thanks.

Btw, Locked states:

Stability: 3 - Locked
Only bug fixes, security fixes, and performance improvements will be accepted.
Please do not suggest API changes in this area; they will be refused.

This is not a bugfix, not a security fix, and not a performance improvement, so this technically contradicts with the Locked state to my understanding. There is an ongoing process of removing the Locked state whatsoever, and this PR should ideally be delayed until that is finished.

/cc @nodejs/ctc

ChALkeR commented Mar 6, 2017

Copy link
Copy Markdown
Member

@rauno56 module was unlocked in #11661, so you can ignore my above comment now =).

rauno56 commented Mar 7, 2017

Copy link
Copy Markdown
Author

Perfect! Thank you. I'll finish it up this week.

rauno56 force-pushed the module-to-internal-errors branch from 42704ed to ddc7f36 Compare March 9, 2017 18:08
module,errors: port module not found error to internal/errors
46b4404 
Keeping assigning error.code = 'MODULE_NOT_FOUND' for backwards
compatibility.
rauno56 force-pushed the module-to-internal-errors branch from ddc7f36 to d1a292f Compare March 21, 2017 19:18
rauno56 force-pushed the module-to-internal-errors branch from d1a292f to f7556d5 Compare March 21, 2017 20:58
jasnell added the blocked PRs that are blocked by other issues or PRs. label Apr 5, 2017
refack removed the blocked PRs that are blocked by other issues or PRs. label Jul 19, 2017

refack commented Jul 19, 2017

Copy link
Copy Markdown
Contributor

I'll follow up

refack self-assigned this Jul 19, 2017

BridgeAR commented Aug 26, 2017

Copy link
Copy Markdown
Member

@refack are you still looking into this / did you open a follow up?

jasnell added the stalled Issues and PRs that are stalled. label Aug 29, 2017

BridgeAR commented Sep 12, 2017

Copy link
Copy Markdown
Member

Closing this due to a long inactivity. @rauno56 I am very sorry that your PR could not land as is and your work is much appreciated nevertheless! Please also feel free to leave a comment if you would like to follow up on this by rebasing or by opening a new PR.

BridgeAR closed this Sep 12, 2017

rauno56 commented Sep 19, 2017 via email

Copy link
Copy Markdown
Author

BridgeAR commented Sep 23, 2017

Copy link
Copy Markdown
Member

@rauno56 I am very sorry to hear that but I understand your frustration that it took long. For a while there were to many open PRs that it became significantly difficult to keep up with all the PRs. This has improved a lot now and I am certain it reviews would be much faster now.

I am reopening this in the hope you would still want to pursue this further. If not, please let us know as well.

BridgeAR reopened this Sep 23, 2017

BridgeAR commented Sep 23, 2017

Copy link
Copy Markdown
Member

Well, I actually just checked and it seems like it was already ported in the meanwhile.

I am very sorry about this @rauno56

BridgeAR closed this Sep 23, 2017
refack removed their assignment Oct 12, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originated in Node.js core. module Issues and PRs related to the module subsystem. stalled Issues and PRs that are stalled.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL