Skip to content

Conversation

@ChALkeR
Copy link
Member

@ChALkeRChALkeR commented Jun 23, 2018

This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the INTST vs INST) in 921fb84 (PR #16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR #18857), as it appeared unused.

This error code never worked or was documented under the mistyped name
ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to
ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix.

Refs: #21440, #21470, #16874, #18857.


This is a part of the fixes hinted by #21470, which includes some tests for error codes usage and documentation and enforces a stricter format.

This is mostly a revert, and specific tests for esm loader actually emitting that error are not included here.
Generic tests for error codes are included in a separate PR: #21470.

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

@ChALkeRChALkeR added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jun 23, 2018
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-botnodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jun 23, 2018
@ChALkeRChALkeRforce-pushed the doc-errcodes-remove-unused2 branch from bd6bb89 to 1a1b056CompareJune 23, 2018 23:39
@ChALkeRChALkeRforce-pushed the doc-errcodes-remove-unused2 branch 2 times, most recently from bbcb61d to 1fcf5e1CompareJune 23, 2018 23:48
@ChALkeRChALkeR added the esm Issues and PRs related to the ECMAScript Modules implementation. label Jun 23, 2018
Copy link
Member

@devsnekdevsnek left a comment

Choose a reason for hiding this comment

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

sorry about that 😃

on a side note, how come our eslint rules didn't pick up throwing a non-internal-error

@ChALkeR
Copy link
MemberAuthor

@devsnek eslint doesn't track that. #21470 does 😉 — that's what picked that up.

@ChALkeRChALkeR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 23, 2018
@jasnell
Copy link
Member

@targos
Copy link
Member

Ping @ChALkeR there is a small conflict.

@ChALkeR
Copy link
MemberAuthor

@targos, sorry, I am not feeling well and have only my phone right now. 🌡️
I will be able to take a look at this later (I suppose today-tomorrow).
Alternatively, I would be happy if anyone could take it from this point.

@targos
Copy link
Member

@ChALkeR sure. I hope you get better soon! I'm happy to take it over. Can I push to your branch?

@ChALkeR
Copy link
MemberAuthor

@targos, thanks!
Yes, of course - I think that left the appropriate flag checked.

@targostargosforce-pushed the doc-errcodes-remove-unused2 branch from 1fcf5e1 to d7fa6c2CompareJune 30, 2018 09:27
@targos
Copy link
Member

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

This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR nodejs#16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR nodejs#18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: nodejs#21440 Refs: nodejs#21470 Refs: nodejs#16874 Refs: nodejs#18857
@targostargosforce-pushed the doc-errcodes-remove-unused2 branch from d7fa6c2 to 75bf6a8CompareJune 30, 2018 12:58
@targos
Copy link
Member

targos commented Jun 30, 2018

@ChALkeR
Copy link
MemberAuthor

@targos Thanks for that again!

@targos
Copy link
Member

No problem!
And trying Windows again... https://ci.nodejs.org/view/All/job/node-test-commit-windows-fanned/19059/

@ChALkeRChALkeR mentioned this pull request Jun 30, 2018
3 tasks
@targos
Copy link
Member

Landed in 7bdc694

@targostargos closed this Jun 30, 2018
targos pushed a commit that referenced this pull request Jun 30, 2018
This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR #16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR #18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: #21440 Refs: #21470 Refs: #16874 Refs: #18857 PR-URL: #21493 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ron Korving <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@ChALkeR
Copy link
MemberAuthor

Node.js v10.x is affected.
v8.x — only after #16874 is backported (if it gets backported).

targos pushed a commit that referenced this pull request Jul 3, 2018
This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR #16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR #18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: #21440 Refs: #21470 Refs: #16874 Refs: #18857 PR-URL: #21493 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ron Korving <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@targostargos mentioned this pull request Jul 3, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.errorsIssues and PRs related to JavaScript errors originated in Node.js core.esmIssues and PRs related to the ECMAScript Modules implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@ChALkeR@nodejs-github-bot@jasnell@targos@ronkorving@benjamingr@lpinca@TimothyGu@devsnek@vsemozhetbyt@trivikr