Gozala / web-encoding

TextEncoder and TextDecoder APIs from Encoding Standard APIs in a universal package
10 stars 6 forks source link

Problems with jest in env=node and env=jsdom #1

Closed Gozala closed 4 years ago

Gozala commented 4 years ago

As per https://github.com/multiformats/js-multibase/issues/66 (quoting below):

Describe the bug When running the test in jest (with either env=jsdom or env=node setting) the TextDecoder or TextEncoder is not a constructor error makes it impossible to write unit tests using this module

To Reproduce This simple test and code fails to run in jest code: https://github.com/Uniswap/uniswap-interface/blob/0aabf4856e6e90041caf4233db4b7defb01574a2/src/utils/contenthashToUri.ts test: https://github.com/Uniswap/uniswap-interface/blob/0aabf4856e6e90041caf4233db4b7defb01574a2/src/utils/contenthashToUri.test.skip.ts

error: TypeError: TextDecoder is not a constructor


    TypeError: TextDecoder is not a constructor

    > 1 | import CID from 'cids'
        | ^
      2 | import { getCodec, rmPrefix } from 'multicodec'
      3 | import { decode, toB58String } from 'multihashes'
      4 |

      at Object.<anonymous> (node_modules/multibase/src/util.js:6:21)
      at Object.<anonymous> (node_modules/multibase/src/base.js:4:24)
      at Object.<anonymous> (node_modules/multibase/src/constants.js:5:14)
      at Object.<anonymous> (node_modules/multibase/src/index.js:9:19)
      at Object.<anonymous> (node_modules/multihashes/src/index.js:10:19)
      at Object.<anonymous> (node_modules/cids/src/index.js:3:12)
      at Object.<anonymous> (src/utils/contenthashToUri.ts:1:1)
      at Object.<anonymous> (src/utils/contenthashToUri.test.ts:1:1)

Expected behavior TextDecoder from the environment (specifically, global for node and window for jsdom jest setting) is successfully loaded

Screenshots image

Desktop (please complete the following information):

  • OS: MacOS
Gozala commented 4 years ago

@moodysalem would you mind submitting a pull request with a test case that illustrates the problem, e.g. by adding test:jest npm script that uses jest with env=jsdom. If that is in place it would be a lot easier for me to fix this.

Gozala commented 4 years ago

I have attempted to create a test to run with jest, but attempt to running it with jest produces following error, which suggest that there's more things involved than just this and jest.

jest ./test/jest.test.js            
 FAIL  test/jest.test.js
  ● Test suite failed to run

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    /Users/gozala/Projects/web-encoding/test/jest.test.js:1
    import { TextEncoder, TextDecoder } from "..";
    ^^^^^^

    SyntaxError: Cannot use import statement outside a module

      at Runtime._execModule (node_modules/jest-runtime/build/index.js:1179:56)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        2.33 s
Ran all test suites matching /.\/test\/jest.test.js/i.
moodysalem commented 4 years ago

https://github.com/Gozala/web-encoding/pull/3 jsdom test fails for a jsdom uint8array compatibility issue, i saw something like this in the jest issues but i couldn't find it

also not sure why it's not failing here. works fine for node. need to extract the create-react-app configuration to reproduce the exact issue

Gozala commented 4 years ago

Thanks for submitting the pull.

jsdom test fails for a jsdom uint8array compatibility issue, i saw something like this in the jest issues but i couldn't find it

I think it's something with how jest implements deep equality check. Fixed that in #4

also not sure why it's not failing here. works fine for node. need to extract the create-react-app configuration to reproduce the exact issue

I think that confirms my suspicion that problem is with wrong module been pulled by the compiler. However we'd need to:

Once that is clear we might get a chance to workout a solution so things just work out of the box. Although there is a chance that problem might lay elsewhere that changes here won't address.

Gozala commented 4 years ago

I have created another test case that use react-scirpts test as the original issue did: https://github.com/Uniswap/uniswap-interface/blob/0aabf4856e6e90041caf4233db4b7defb01574a2/package.json#L86

Oddly enough I can reproduce the problem locally, but CI does not seem to exhibit the problem 🥺

Gozala commented 4 years ago

Spend some time debugging this. Turns out Jest isn't using node module loader but does it's own module loading which also overrides node global with jsdom window, that does not have TextEncoder and TextDecoder. And I'm not even sure what this supposed to do:

image image

So yeah there is no TextEncoder / TextDecoder exports there, but I have no clue why instead of module source it end up with just exporting a module name string.

Gozala commented 4 years ago

I think jest gets confused because it expects ES module, but is encountering commonjs module.

Gozala commented 4 years ago

Turns out not using .cjs is breaking other things on node side, see https://nodejs.org/api/esm.html 😭

Gozala commented 4 years ago

Relevant issue https://github.com/facebook/create-react-app/issues/8754 which was closed it seems

Gozala commented 4 years ago

@moodysalem I'm afraid there does not seems to be a way to fix this without breaking anything else, because node requires .cjs extension to make dual ESM / CJS packages. Which seems to have been reported to react-scripts more than once, but they seem to be locked or closed.

It also appears that react-scripts doesn't support jest's moduleFileExtensions setting which would have addressed the issue.

However new react-scripts seems to have updated jest dependency, which in turn seems to have included cjs into default moduleFileExtensions and I have verified that with react-scripts@4.0.0-next.77 or more simplyreact-scripts@next everything works as expected https://github.com/Gozala/web-encoding/pull/6

@moodysalem if you can upgrade to react-scripts@4.0.0-next.77 I think that would be a best solution & hopefully it will get released soon to make all this a non-issue.