-
-
Notifications
You must be signed in to change notification settings - Fork 190
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
chore: minor, non-functional changes #876
Conversation
* remove `actionThunks` variable from `extractDataFromModel` which was no longer in use
* this is for developers who are configured to use private registries at work. configuring at the project level will override any global configuration for commands like `npm publish`.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -26,7 +26,6 @@ export default function extractDataFromModel( | |||
const _aCD = {}; | |||
const _aC = {}; | |||
const _aRD = {}; | |||
const actionThunks = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, this was never being used. All tests still pass.
const logEventListenerError = (type, err) => { | ||
// eslint-disable-next-line no-console | ||
console.log(`Error in ${type}`); | ||
// eslint-disable-next-line no-console | ||
console.log(err); | ||
}; | ||
|
||
export const handleEventDispatchErrors = | ||
(type, dispatcher) => | ||
(...args) => { | ||
try { | ||
const result = dispatcher(...args); | ||
if (isPromise(result)) { | ||
result.catch((err) => { | ||
logEventListenerError(type, err); | ||
}); | ||
} | ||
} catch (err) { | ||
logEventListenerError(type, err); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the code a bit more DRY by replacing repeated code with shared util.
@@ -7,7 +7,7 @@ const noopStorage = { | |||
removeItem: () => undefined, | |||
}; | |||
|
|||
const getBrowerStorage = (storageName) => { | |||
const getBrowserStorage = (storageName) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For devs that use private NPM registries. Avoid conflicts with commands like npm publish
// Register the thunk handler | ||
set(path, actionThunks, def.thunkHandler); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we don't have tests covering this? IDK what actionThunks
are
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested this branch against our codebase, and we get 0 failures - so I guess this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is related to this change: #876 (comment). The only place actionThunks
was ever being used was here, the thunkHandler
was being set on the object but then the object never got passed along to anything else. And if all the tests still pass, I'm thinking it's fine. Ctrl + Shift + F also turned up no other references to actionThunks
. Obviously this isn't definitive because it could be passed to a func that calls it something else, but if deleting this line doesn't break anything else I think we're good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff 👌
handleEventDispatchErrors
to lib