astagi / workerloader-jest-transformer

🃏 Jest transformer for Webpack worker-loader
MIT License
15 stars 3 forks source link

Fix for TypeScript, adding some tests and fixing typos #4

Closed visvirial closed 3 years ago

visvirial commented 3 years ago

In this PR, I fixed some transformation errors on TypeScript, added WebWorker tests and fixed some other minor issues.

Known issues (should be fixed in the future):

(Note: if you encountered an error by executing Jest, please clear your cache using jest --clearCache and / or just using jest --no-cache)

codecov-commenter commented 3 years ago

Codecov Report

Merging #4 (a771f94) into master (636536d) will increase coverage by 0.31%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #4      +/-   ##
==========================================
+ Coverage   97.56%   97.87%   +0.31%     
==========================================
  Files           3        3              
  Lines          41       47       +6     
  Branches        3        6       +3     
==========================================
+ Hits           40       46       +6     
  Misses          1        1              
Impacted Files Coverage Δ
utils/stripper.js 100.00% <100.00%> (ø)
utils/wrapper.js 100.00% <0.00%> (ø)
lib/baseworker.js 96.55% <0.00%> (+0.12%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 636536d...a771f94. Read the comment docs.

astagi commented 3 years ago

@visvirial this PR looks awesome! thank you so much! Yeah I opened this repo as a little experimental module, I'm very happy to see contributions and interest on it!

visvirial commented 3 years ago

When will it be merged?

astagi commented 3 years ago

@visvirial I'll check it during the weekend! I'll merge it on Monday and then draft a new release on NPM! Sorry but I'm very busy at the moment! Thank you for the awesome work, let you know asap

astagi commented 3 years ago

@visvirial in the meantime please add yourself to CONTRIBUTORS

visvirial commented 3 years ago

That's okay. I'm not in a hurry but I just wanted to make sure that my PR is surely in a review. Have a nice weekend!

astagi commented 3 years ago

Again, awesome work @visvirial , thank you so much! Version 0.0.4 is now available on npm

  • The files imported in the *.worker.ts should be a JavaScript file and cannot be a TypeScript file.

Mmh that's interesting, it's been a long time since I looked at this code last time, also I never tried importing typescript files inside the worker, what's wrong with them?

  • If there is a comment or something which contains the string import, ./utils/stripper.js considers it as a module imports, and outputs a wrong JS string.

    • Try adding a comment // This will import mylibrary. to ./__test__/echoworker.worker.js somewhere. This will throw a SyntaxError.
    • The importRe regular expression in ./utils/stripper.js should be sophisticated, and / or some syntax interpretation should be applied.

You're right, import is a reserved word but it can be used in a comment or in a string, I think that a more sophisticated regular expression may intercept it, without diving into syntax interpretation (I suppose you're talking about using AST, right?)

visvirial commented 3 years ago

Yes, I mean some AST should be employed to be more strictly.

However, since that import statement will appear at the beginning of lines, how about appending the letter '^' to the beginning of importsRe in utils/stripper.js? (Note: also dynamic imports like const foo = await import('foo') shouldn't be matched.)

visvirial commented 3 years ago

Uh, this won't work for import statements in a multiple line comments:

/**
import bar from 'bar';
*/

This will match the regex...


The most reliable and straight forward way, I think, is to force *.worker.ts to include /* __SEPARATOR__ */ as in https://github.com/visvirial/jest-webworker