Skip to content
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

errors: fix ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK #21493

Closed
wants to merge 1 commit into from

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR 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

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

@devsnek devsnek 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
Member Author

ChALkeR commented Jun 23, 2018

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

@ChALkeR ChALkeR 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

jasnell commented Jun 29, 2018

@targos
Copy link
Member

targos commented Jun 30, 2018

Ping @ChALkeR there is a small conflict.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 30, 2018

@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

targos commented Jun 30, 2018

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

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 30, 2018

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

@targos targos force-pushed the doc-errcodes-remove-unused2 branch from 1fcf5e1 to d7fa6c2 Compare June 30, 2018 09:27
@targos
Copy link
Member

targos commented Jun 30, 2018

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
@targos targos force-pushed the doc-errcodes-remove-unused2 branch from d7fa6c2 to 75bf6a8 Compare June 30, 2018 12:58
@targos
Copy link
Member

targos commented Jun 30, 2018

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 30, 2018

@targos Thanks for that again!

@targos
Copy link
Member

targos commented Jun 30, 2018

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

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

targos commented Jun 30, 2018

Landed in 7bdc694

@targos targos 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
Member Author

ChALkeR commented Jun 30, 2018

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]>
@targos targos mentioned this pull request Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.