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

fix(babel): keep function names to improve stack traces in tests #189

Merged
merged 2 commits into from Jan 10, 2021

Conversation

mpeyper
Copy link
Contributor

@mpeyper mpeyper commented Jan 9, 2021

What:

Preserve function names when eliminating dead code in tests to enable nicer stack traces when errors occur.

Why:

Fixes #188

How:

I used the isTest flag to enable the keepFnName option of babel-plugin-minify-dead-code-elimination.

Checklist:

  • Documentation N/A
  • Tests N/A
  • Ready to be merged

@codecov
Copy link

codecov bot commented Jan 9, 2021

Codecov Report

Merging #189 (57fa2d2) into master (2e7e312) will decrease coverage by 21.52%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #189       +/-   ##
===========================================
- Coverage   85.61%   64.08%   -21.53%     
===========================================
  Files          17       21        +4     
  Lines         424      568      +144     
  Branches      162      214       +52     
===========================================
+ Hits          363      364        +1     
- Misses         54      165      +111     
- Partials        7       39       +32     
Impacted Files Coverage Δ
src/config/babelrc.js 52.27% <33.33%> (-1.39%) ⬇️
src/scripts/build/babel.js 0.00% <0.00%> (ø)
src/scripts/build/rollup.js 0.00% <0.00%> (ø)
src/scripts/build/index.js 0.00% <0.00%> (ø)
src/scripts/typecheck.js 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e7e312...57fa2d2. Read the comment docs.

@mpeyper
Copy link
Contributor Author

mpeyper commented Jan 9, 2021

I'm having some second thoughts on this. This will only help the case where a project is using kcd-scripts and running their own tests to see the function names of their own code. For my case, I'm writing a library to be used in tests, so if I build my code with kcd-scripts to publish to npm the function names will still be removed.

Digging through the history a bit, it looks like this plugin came about as part of an UMD feature, which sort of makes sense to me as I'd expect the consumer of the package to deal with the bundling, minifying, treeshaking, etc. when using it from NPM. I'm wondering if should only be added to the config if:

  1. isUMD is true
  2. and/or BUILD_MINIFY environment variable is set to true

I've also discovered that this issue is killing the display names of React HOCs:

const withValue = (value) => (Component) => {
  const WithValue = (props) => (
    <Component {...props} value={value} />
  )
  return WithValue
}

becomes:

const withValue = value => Component => {
  return props => /*#__PURE__*/React.createElement(Component, (0, _extends2.default)({}, props, {
    value: value
  }));
};

(although it could be argued that HOCs should be setting their display name explicitly).

@kentcdodds
Copy link
Owner

Yeah, let's only enable the minification plugins when BUILD_MINIFY is true 👍

@@ -81,7 +81,7 @@ module.exports = () => ({
? require.resolve('babel-plugin-transform-inline-environment-variables')
: null,
[require.resolve('@babel/plugin-proposal-class-properties'), {loose: true}],
require.resolve('babel-plugin-minify-dead-code-elimination'),
['babel-plugin-minify-dead-code-elimination', {keepFnName: isTest}],
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add:

const isMinify = parseEnv('BUILD_MINIFY', false)

Above, and then change this to:

Suggested change
['babel-plugin-minify-dead-code-elimination', {keepFnName: isTest}],
isMinify ? require.resolve('babel-plugin-minify-dead-code-elimination') : null,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Also, I didn't even notice I have removed the require.resolve in my first commit 😅

P.S. I would have taken your suggestion if it were possible to add the parseEnv at the same time.

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Awesome, thanks 👍

@kentcdodds kentcdodds merged commit 67a5323 into kentcdodds:master Jan 10, 2021
@kentcdodds
Copy link
Owner

@all-contributors please add @mpeyper for code

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @mpeyper! 🎉

@github-actions
Copy link

🎉 This PR is included in version 7.5.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

babel-plugin-minify-dead-code-elimination removes function names
2 participants