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 the crash when hashing an async function #90

Merged
merged 3 commits into from Feb 10, 2020

Conversation

whatisaphone
Copy link
Contributor

Fixes #67.

This code will no longer crash:

objectHash(async function foo () {})

@whatisaphone
Copy link
Contributor Author

whatisaphone commented Dec 12, 2019

I had to add a workaround for the old browserify build system. There's already one workaround there, so a second didn't seem too bad. The workaround only applies to the bundled code run with Karma. The tests run from Node still run with the original non-bundled code.

The tests pass for everything except Node 6, which didn't support async functions. Node 6 reached end-of-life back in April. Maybe it's time to drop support for it?

@whatisaphone
Copy link
Contributor Author

Node 6 reached EOL last decade. The tests pass for Node >= 8.

That probably means a major version bump. There are no other breaking changes.

@agilgur5
Copy link

agilgur5 commented Feb 5, 2020

@addaleax any word on this? Currently facing this problem in TSDX in several issues: jaredpalmer/tsdx#294 , jaredpalmer/tsdx#358 (comment) , jaredpalmer/tsdx#278 (comment) , and a few in jaredpalmer/tsdx#379

TSDX is using object-hash via rollup-plugin-typescript2, where there's an old issue around this (ezolenko/rollup-plugin-typescript2#105) and the current fix/workaround is to disable caching, which is very suboptimal. Would be great to actually fix this bug instead of having a hacky workaround!

You had said you would look into this back then in #68 (comment) and then there was never an update (@wessberg moved on to make a competing plugin that seems to have some custom hashing? or something).

This approach should also work for async generator functions as they come out as 'asyncgeneratorfunction'.
A similar approach is mentioned in tc39/proposal-async-await#78 (comment) for programmatic detection of async functions as well (the cons aren't relevant to this use case -- toString and everything still work).

@arvinsim
Copy link

@agilgur5 I was happily using TSDX until I tried to use rollup-plugin-visualizer to measure my bundle sizes when I got this error. While it is not urgent right now, it would be nice if they can fix this seemingly annoying issue.

@agilgur5
Copy link

@arvinsim It is one of the top issues in TSDX, particularly with how frequently it's referenced. A good chunk of people are definitely using the rpts2 workaround that disables caching, which is a poor developer experience. That's why I chimed in here as I believe it's pretty high priority for TSDX to fix.

And unfortunately it can't be monkey-patched and can't easily be patched with a fork as it's a transitive dependency 😕

@addaleax
Copy link
Collaborator

Yeah, all I need to do is to fix this test up so that instead of the last commit there is actual support for Node.js 6 … will do that now

Co-authored-by: Anna Henningsen <anna@addaleax.net>
@addaleax addaleax merged commit e090f7e into puleos:master Feb 10, 2020
@addaleax
Copy link
Collaborator

Should be fixed in v2.0.2 now

@agilgur5
Copy link

Thanks so much for the quick response & release @addaleax !!

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

Successfully merging this pull request may close these issues.

Doesn't support async functions
4 participants