Refactor beta branch for a bit more consistency #528
Conversation
…tions and arrow functions
Codecov Report
@@ Coverage Diff @@
## beta #528 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 12 12
Lines 186 189 +3
Branches 27 27
=========================================
+ Hits 186 189 +3
Continue to review full report at Codecov.
|
src/core/index.ts
Outdated
| // If the function name does not get used before it is returned, | ||
| // it seems to vanish in nodejs and does not appear in stack traces. | ||
| // This dummy usage works around that. | ||
| const _name = renderHook.name // eslint-disable-line @typescript-eslint/no-unused-vars |
There was a problem hiding this comment.
This is gross and I have no idea why it is necessary, but in node, if I've got function that returns a function like so:
function createFunc() {
function func() {
}
return func
}then:
console.log(createFunc.name) // 'createFunc'
console.log(createFunc().name) // ''But if I use func.name inside createFunc like so:
function createFunc() {
function func() {
}
const name = func.name // not returned, just used
return func
}then:
console.log(createFunc.name) // 'createFunc'
console.log(createFunc().name) // 'func'In the chrome console, both example will return func for the second log. It's very weird.
There was a problem hiding this comment.
That is really weird. Can't seem to find much online either how to potentially solve.
There was a problem hiding this comment.
So I've discovered that this isn't a node thing, but something in the compilation. I've narrowed it down to babel (not TS and not something in jest) as if I use kcd-scripts to compile a javascript only file, this
export function createFunc1() {
function func() {
}
return func
}
export function createFunc2() {
function func() {
}
const name = func.name // not returned, just used
return func
}becomes
"use strict";
Object.defineProperty(exports, "__esModule", {
value: true
});
exports.createFunc1 = createFunc1;
exports.createFunc2 = createFunc2;
function createFunc1() {
return function () {};
}
function createFunc2() {
function func() {}
func.name; // not returned, just used
return func;
}The name in createFunc1 has been scrubbed!
Now I just have to work out which plugin is causing this.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks @jpeyper. I've raised an issue in kcd-scripts about this.
There was a problem hiding this comment.
I'll keep an eye on the other issue, in the meantime, we have this workaround 🤷
Co-authored-by: Jonathan Peyper <[email protected]>
|
🎉 This PR is included in version 5.0.0-beta.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
@allcontributors please add @jpeyper for review, code |
|
I've put up a pull request to add @jpeyper! 🎉 |
What:
More consistency across the codebase.
Why:
It started with me wanting to have a named function as the result of
createRenderHookrather than the anonymous arrow function it is returning now to improve stack traces when debugging, but the more I looked the more I noticed we had a real mix of regular functions and arrow functions through the codebase, even within the same file. This led me down a rabbit hole of a few other things that weren't quite right, names default exports and a few variable names.How:
For functions I followed the rules:
We only had a single
default exportleft in the codebase. There has been a growing movement in the community away from them and to just use named exports instead. It's become my personal preference for all exports over the last few months.Finally,
testHookPropswas previously renamed torendererPropsin all but one of the renderers (native), so I have changed that on now as well. Also, I renamedtestHooktotestHarnessin all renderers as it is the result of thecreateTestHarnesshelper.Checklist:
There is also a lot of inconsistency in the types with respect to
interfacevstype, but I'll do another PR for that as I want change something else and #527 moves one of the types I need to change so I'll wait for that one to be merged also.