Skip to content
This repository has been archived by the owner on Sep 9, 2021. It is now read-only.

fix: set a name to exported fn #299

Merged
merged 5 commits into from
Dec 2, 2020

Conversation

ramasilveyra
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

This can help debugging since error stack traces will have names.

And also removes the warning of anonymous functions next.js https://nextjs.link/codemod-ndc from.

Breaking Changes

None

Additional Info

None

this can help debugging and also removes the warning https://nextjs.link/codemod-ndc from next.js
@codecov
Copy link

codecov bot commented Nov 16, 2020

Codecov Report

Merging #299 (38888c6) into master (464bf30) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
+ Coverage   76.02%   76.19%   +0.16%     
==========================================
  Files           6        6              
  Lines         146      147       +1     
  Branches       51       51              
==========================================
+ Hits          111      112       +1     
  Misses         30       30              
  Partials        5        5              
Impacted Files Coverage Δ
src/utils.js 86.84% <100.00%> (+0.35%) ⬆️

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 464bf30...38888c6. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

What is the problem? Can you provide example

@ramasilveyra
Copy link
Contributor Author

@evilebottnawi next.js logs this when every worker is built, which is a bit noisy:

../lib/workers/some.worker.ts
Anonymous function declarations cause Fast Refresh to not preserve local component state.
Please add a name to your function, for example:
Before
export default function () { /* ... */ }
After
export default function Named() { /* ... */ }
A codemod is available to fix the most common cases: https://nextjs.link/codemod-ndc

The code of this log is here https://github.com/vercel/next.js/blob/8221c180a55828b39a7e1e01811365bfaaa0dd19/packages/next/build/babel/plugins/no-anonymous-default-export.ts#L59-L68.

But adding a function name in this lib it can also help while debugging errors, since stack trace errors will have names.

@alexander-akait
Copy link
Member

No sure it will fix the problem with hot reloading, but I can merge it

package.json Outdated
"schema-utils": "^3.0.0",
"loader-utils": "^2.0.0"
"loader-utils": "^2.0.0",
"lodash.camelcase": "^4.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid using this module, it is deprecated, we don't need extra module for simple logic

Copy link
Contributor Author

@ramasilveyra ramasilveyra Nov 26, 2020

Choose a reason for hiding this comment

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

@evilebottnawi Sorry for my delay.

I've tried to use simpler solutions like https://stackoverflow.com/a/2970667, but they can fail with RangeError.

So since lodash.camelcase is deprecated I have remplace it with camelcase.

camelcase:

If still doesn't work, I can try inlining this code https://github.com/sindresorhus/camelcase/blob/master/index.js here.

package.json Outdated Show resolved Hide resolved
@ramasilveyra ramasilveyra force-pushed the fix/fn-names branch 2 times, most recently from eba886e to b218745 Compare November 26, 2020 20:32
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Why just don't use ${typeOfWorker}_fn name without any additional logic?

@ramasilveyra
Copy link
Contributor Author

ramasilveyra commented Dec 1, 2020

@alexander-akait by typeOfWorker do you mean workerConstructor?

let workerConstructor;
let workerOptions;
if (typeof options.worker === 'undefined') {
workerConstructor = 'Worker';
} else if (typeof options.worker === 'string') {
workerConstructor = options.worker;
} else {
({ type: workerConstructor, options: workerOptions } = options.worker);
}

I can make that change, that would still solve the issue with noisy logs from next.js! The only difference is that unique function names can help while debugging.

@alexander-akait
Copy link
Member

I don't think you need unique function here because you already have stack trace in console with link on file, line and column

@ramasilveyra
Copy link
Contributor Author

@alexander-akait done!

@alexander-akait alexander-akait merged commit 15cf407 into webpack-contrib:master Dec 2, 2020
@alexander-akait
Copy link
Member

Thanks

@ramasilveyra ramasilveyra deleted the fix/fn-names branch December 2, 2020 17:09
@ramasilveyra
Copy link
Contributor Author

Thanks for your time @alexander-akait !

TheLD6978 pushed a commit to TheLD6978/worker-loader that referenced this pull request Apr 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants