[GH-788] Made the check to determine whether the current web-worker is owned by PapaParse more reliable

[flakey-bit]

This fixes #788

Previously, the assumption was if we're in a web-worker AND the worker was loaded from a blob, the worker must be ours.

[pokoli]

Hi @flakey-bit,

Thanks for working on this. I see there is an issue on your patch that match our test suite broken.
Could you have a look in order to fix it? TIA

[flakey-bit]

Hi @flakey-bit,

Thanks for working on this. I see there is an issue on your patch that match our test suite broken. Could you have a look in order to fix it? TIA

@pokoli

Can you link me to the test-suite build failure? never mind, I found it.

NB: I found there was an issue with my implementation so I've had another go at fixing it 🤞 - I've force-pushed to my branch, which should have updated the PR. See latest build queued here.

[flakey-bit]

@pokoli

I've added a test case to cover this scenario. Please take a look

[pokoli]

Hi @flakey-bit,

Test failed on node12 which is unsuported by now.
I removed the support of node 12 on master branch, could you please rebase it?
If test passes I will merge it.

[flakey-bit]

@pokoli rebase is done

[pokoli]

@flakey-bit Tests are still broken, could you please have a look?

You can run them locally with: npm test

[flakey-bit]

I'll take a look tomorrow. Sorry for taking up your time!

…

On Fri, 10 Jun 2022, 22:39 Sergi Almacellas Abellana, < ***@***.***> wrote: @flakey-bit <https://github.com/flakey-bit> Tests are still broken, could you please have a look? You can run them locally with: npm test — Reply to this email directly, view it on GitHub <#937 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBVTAKGN4AJKUM6CQW4MILVOMLMNANCNFSM5YE26S3A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[flakey-bit]

@pokoli have force-pushed again.

npm test is clean now (as is running npm run test-node and npm run test-mocha-headless-chrome separately.

Note I've included an additional to commit to allow the tests to use latest ECMAScript syntax (does not affect the targeting of the main library).

[flakey-bit]

@Silic0nS0ldier can you also take a look at this? The approach will likely have implications for the move to ES6 modules (#875)

In particular, see the test case that this PR adds.

[pokoli]

@flakey-bit why is required to add the latest ECMAScript syntax? Does the test work without that?

[flakey-bit]

@pokoli the test is currently written using async keyword, arrow functions, template strings etc.

Could rewrite the test not to use such features, but would probably make it harder to understand.

[Silic0nS0ldier]

@flakey-bit I've lost quite a bit of context since opening #875, but I don't see any reason for this to cause problems for ES6.

Not sure how relevant it will be here, but I did some work to solve the same problem with NodeJS worker threads some time ago. https://github.com/developit/web-worker/blob/9ee077b6c5df0acaf5d83d89e2087bd3a3b82b97/src/node.js#L138-L142

[flakey-bit]

@pokoli any chance of getting a new release out to NPM?

[pokoli]

@flakey-bit I will prefer to wait a little if there is something else to be included on the release.

[flakey-bit]

@pokoli it would be awesome to get a release out if it's not too much trouble

I know it's a bit selfish of me, but part of the reason I picked up #788 was to allow us to integrate PapaParse into our application without resorting to patch-package 🙂