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

- partially undoing lodash change #333

Closed
wants to merge 2 commits into from
Closed

Conversation

ezolenko
Copy link
Owner

@ezolenko ezolenko commented May 30, 2022

Partially undoing recent lodash change (#328), _.each() semantics is apparently different from Array.forEach() -- during the build it created "undefined" folder with bunch of empty files.

@ezolenko ezolenko requested a review from agilgur5 May 30, 2022 17:26
src/tscache.ts Outdated Show resolved Hide resolved
@agilgur5
Copy link
Collaborator

agilgur5 commented May 31, 2022

during the build it created "undefined" folder with bunch of empty files.

ah my bad, thanks for catching this and the exact line too.

I did see this folder once or twice but thought that was because of all the other changes and investigations I was doing and just a temporary thing I broke -- turns out it wasn't temporary 😅
It also doesn't pop up during testing because, well, there are no tests for this yet

_.each() semantics is apparently different from Array.forEach()

The semantics are only slightly different in that _.each handles falsey values, while .forEach doesn't -- but .forEach doesn't need to, as you can only use it on arrays anyway.

Another reason why I think it's a problem with this instead

Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

See my suggested change to the line that causes this -- that's the only one that needs fixing and it's not a forEach issue, it's because of how I rewrote it (removing the anonymous arrow function). With that fix in, there should be no other issues with any lodash replacement.

So if we can just change only that line, think this would be good to go!

src/tscache.ts Outdated Show resolved Hide resolved
@agilgur5 agilgur5 added the kind: regression Specific type of bug -- past behavior that worked is now broken label May 31, 2022
Co-authored-by: Anton Gilgur <agilgur5@gmail.com>
@ezolenko ezolenko closed this Jun 1, 2022
ezolenko added a commit that referenced this pull request Jun 1, 2022
@ezolenko
Copy link
Owner Author

ezolenko commented Jun 1, 2022

Right, I committed that change directly 4310873

@ezolenko ezolenko deleted the fix_for_undefined_dir branch June 1, 2022 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: regression Specific type of bug -- past behavior that worked is now broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants