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

Hide internal @babel/core functions in config errors #11554

Merged
merged 13 commits into from Aug 27, 2022

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 12, 2020

Q                       A
Fixed Issues? Closes #11540
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This PR introduces some logic to hide @babel/core internals in error stack traces for errors coming from outside @babel/core itself (for example, coming from config files or plugins/presets): @babel/core introduces a lot of frames (and all duplicated, because of how we use gensync), which are not really useful.

Unexpected errors thrown inside @babel/core itself still have the complete stack trace, because they are bugs and we need debug info.

If you want to see the old stack traces, you can replace const SUPPORTED = !!Error.captureStackTrace; with const SUPPORTED = false; and run the tests.

@nicolo-ribaudo nicolo-ribaudo added PR: Polish 💅 A type of pull request used for our changelog categories area: errors labels May 12, 2020
@babel-bot
Copy link
Collaborator

babel-bot commented May 12, 2020

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52794/

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 686dfa8:

Sandbox Source
hopeful-cache-0dr69 Configuration
sad-hooks-tow69 Configuration
thirsty-goodall-2d6el Issue #11540

@liuxingbaoyu
Copy link
Member

This PR looks great! The current stack provides little useful information.

@nicolo-ribaudo nicolo-ribaudo force-pushed the hide-stack branch 2 times, most recently from 2fb805c to e69b896 Compare August 24, 2022 14:42
@nicolo-ribaudo
Copy link
Member Author

@liuxingbaoyu Since you use windows, when you have time could you clone this PR, run yarn jest babel-core/test/errors-stacks and see why the tests are not throwing when they should? 🙏

@liuxingbaoyu
Copy link
Member

This is the same as #14671 (comment), by the way I also found a typo there although it works fine.😂

@liuxingbaoyu
Copy link
Member

liuxingbaoyu commented Aug 24, 2022

To be honest, the CI exception from the fork repo scares me.😅
This is extremely strange, I often have similar problems too in my personal repo, but almost none in the main repo.😕

Co-authored-by: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com>
@nicolo-ribaudo
Copy link
Member Author

Thank you! 😄

@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review August 25, 2022 08:26
}

if (!options) throw new Error(`${filepath}: No config detected`);
if (!options) throw new ConfigError(`No config detected`, filepath);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe there is some lint rule to make it a normal string, but in this PR I think it's ok.🤫

Copy link
Contributor

Choose a reason for hiding this comment

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

There's @typescript-eslint/quotes, which can be configured with allowTemplateLiterals: false to avoid unnecessary template literals (despite the name, it does not forbid all usage of template literals). To be compatible with Prettier, avoidEscape: true must also be set (even if you extend eslint-config-prettier, individual rule configurations take precedence).

function (this: any) {
setupPrepareStackTrace();
return fn.apply(this, arguments);
} as any as Fn,
Copy link
Contributor

@JLHwung JLHwung Aug 25, 2022

Choose a reason for hiding this comment

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

The type casting can be avoided if we parameterize the function's input and output.

export function beginHiddenCallStack<T, A extends any[], R>(
  fn: (this: T, ...args: A) => R,
) {
  if (!SUPPORTED) return fn;

  return Object.defineProperty(
    function (this: T, ...args: A) {
      setupPrepareStackTrace();
      return fn.apply(this, args);
    },
    "name",
    { value: STOP_HIDNG },
  );
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We still need to type cast somewhere, because the functions that we receive in the config have type Function which is not compatible with the "explicit" function types. However, I moved them to the config validation logic so that the "internals" are properly typed.

let newTrace = [];

const isExpected = expectedErrors.has(err);
let status = isExpected ? "hiding" : "unknown";
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can you restrict the typing here? It seems to me status can only be "hiding" | "showing" | "unknown".

* injcectVirtualStackFrame(error, filename) function: those are added on the top
* of the existig stack, after hiding the possibly hidden frames.
* In the example above, if we called injcectVirtualStackFrame(error, "h") on the
* expected error thrown by c(), it's shown call stack would have been "h, e, f".
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* expected error thrown by c(), it's shown call stack would have been "h, e, f".
* expected error thrown by c(), its shown call stack would have been "h, e, f".

May extend the example a bit, for example, what will be the shown call stack if we called injcectVirtualStackFrame(error, "h") and injcectVirtualStackFrame(error, "i")?

@nicolo-ribaudo nicolo-ribaudo merged commit 32328de into babel:main Aug 27, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the hide-stack branch August 27, 2022 09:25
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Nov 27, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: errors outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: core PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@babel/core parseSync does not handle exclude in config
5 participants