Skip to content

Fix the crash when hashing an async function#90

Merged
addaleax merged 3 commits into
puleos:masterfrom
whatisaphone:fix/async-function
Feb 10, 2020
Merged

Fix the crash when hashing an async function#90
addaleax merged 3 commits into
puleos:masterfrom
whatisaphone:fix/async-function

Conversation

@whatisaphone

Copy link
Copy Markdown
Contributor

Fixes #67.

This code will no longer crash:

objectHash(async function foo () {})

@whatisaphone

whatisaphone commented Dec 12, 2019

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

agilgur5 commented Feb 5, 2020

Copy link
Copy Markdown

@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
Copy Markdown

@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
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
Collaborator

Should be fixed in v2.0.2 now

@agilgur5

Copy link
Copy Markdown

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