search

ngduc / react-tabulator

React Tabulator is based on tabulator - a JS table library with many advanced features.
https://codesandbox.io/s/0mwpy612xw?module=/src/components/Home.js
MIT License
368 stars 84 forks source link

Jest Test in Create-React-App broken #215

Open Firionus opened 3 years ago

Firionus commented 3 years ago

Title

Environment Details

Long Description To reproduce:

Result: 

(node:9100) UnhandledPromiseRejectionWarning: TypeError: tabulator_tables_1.default is not a constructor
(node:9100) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
(node:9100) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
 PASS  src/App.test.tsx
  √ renders learn react link (45 ms)

Expected Result: No Warnings

Test can even be made to fail by making it async: 

test('renders learn react link', async () => {
  render(<App />);
  const linkElement = screen.getByText(/learn react/i);
  expect(linkElement).toBeInTheDocument();
  await waitFor(() => {
    expect(screen.queryByText("nowhere to be found")).not.toBeInTheDocument();
  });
});

yielding the test result:

 FAIL  src/App.test.tsx
  × renders learn react link (46 ms)

  ● renders learn react link

    TypeError: tabulator_tables_1.default is not a constructor

      at default_1.<anonymous> (node_modules/react-tabulator/lib/ReactTabulator.js:111:25)
      at step (node_modules/react-tabulator/lib/ReactTabulator.js:57:23)
      at Object.next (node_modules/react-tabulator/lib/ReactTabulator.js:38:53)
      at fulfilled (node_modules/react-tabulator/lib/ReactTabulator.js:29:58)

Unfortunately, the test works in Codesandbox (https://codesandbox.io/s/vr3in).

Workaround

No idea right now.

Related Issue https://github.com/ngduc/react-tabulator/issues/204

I tried npm cache clean --force, followed by deleting node_modules and npm install, but npm test still gives the same FAIL.

ngduc commented 3 years ago

unit tests work find for commits in master and local, see "yarn test" in: https://github.com/ngduc/react-tabulator/blob/master/.github/workflows/node.js.yml

I think this error is from an older version "tabulator_tables_1.default is not a constructor", upgrade react-tabulator to the lastest version should fix it.

Firionus commented 3 years ago

With the current release, I can still reproduce on another machine:

CI aside, can't you reproduce on your machine?

ngduc commented 3 years ago

Yes, there is some issue when it runs in local, sometimes it fails to launch puppeteer and got stuck there. You may want to kill node processes and try to run the unit tests again. Updated here: https://github.com/ngduc/react-tabulator/blob/master/docs/development.md#unit-tests

mike-lischke commented 3 years ago

Here we are, half a year later and with react-tabulator version 0.15.0, and I still see this issue, while writing Jest tests. Since half of my app depends on (react-)tabulator, I'm pretty much lost when it comes to writing tests for that part.

Is there any progress happening meanwhile? Or is a workaround available? Since I also had trouble with monaco-editor, I tried to apply the same trick, first to tabulator-tables then to react-tabulator:

        "transformIgnorePatterns": [
            "/node_modules/(?!monaco-editor)/",
            "/node_modules/(?!react-tabulator)/"
        ],
        "moduleNameMapper": {
            "^monaco-editor$": "monaco-editor/esm/vs/editor/editor.api",
            "^react-tabulator$": "react-tabulator/lib/index.js",
        }

but that didn't help. Note: react-tabulator works without trouble in the application (many thanks for that, btw.!), just not in Jest tests.

ngduc commented 3 years ago

Sorry about this intermittent puppeteer launching issue in local. I think upgrading to a newer version of puppeteer may help solve this one. If you could help upgrade it, it'd be great. I will try to do that also.

timiscoding commented 3 years ago

I'm pretty sure the issue I have is related to this and https://github.com/ngduc/react-tabulator/issues/204 so rather than write a new bug report, I'll just add my thoughts here.

I've been looking at adding a iOS app to our desktop webapp using the Meteor Cordova framework which essentially runs in a webview served over a local web server and I was seeing an error like Unhandled Promise Rejection: TypeError: undefined is not a constructor (evaluating 'new tabulator_tables_1["default"]') from one of our components that used React-tabulator which was odd seeing as our desktop app using the same code was working fine.

Looking in the network tab in devtools I discovered that the only difference was in the way the tabulator-tables dependency was being loaded. Specifically, desktop safari was loading {"module": "dist/js/tabulator.es2015.js"} while in iOS safari, it was loading {"main": "dist/js/tabulator.js"} in package.json. Apparently, the package author added ESM support from 4.8.0 if you read the other GH issue I linked above.

But why was it loading different modules? To answer that question you have to understand that Meteor builds 2 separate bundles, one for modern web browsers called web.browser and the other for older browser support called 'web.browser.legacy'. When building for mobile aka web.cordova it's the same as the legacy build. So in desktop, it used the newer ESM module but in mobile, it was falling back to CJS.

So I tried forcing Cordova to use the ESM module by cloning the tabulator-tables repo into my project to force Meteor to build it. I updated the main field in package.json to point to tabulator.es2015.js and lo and behold it worked. Ugly workaround but it worked.

Next I looked at why this error was occurring in the first place. Turns out, after much hair pulling that there's a bunch of interoperability issues with ESM and CJS modules. React-tabulator uses ESM and when it tries to import tabulator.js, ie. the CJS module from tabulator-tables, it blows up.

The solution? TypeScript has the esModuleInterop compiler option that smooths out default CJS imports, precisely what React-tabulator is doing:

https://github.com/ngduc/react-tabulator/blob/43a883dc23975860c2c3dae4945ea9e3ff84e572/src/ReactTabulator.tsx#L8

Tested this out by running the repro in this issue and npm linking to a React-tabulator 0.15.0 rebuild: 

// package.json
build: "npm run clean && tsc --esModuleInterop --outDir ./lib --jsx react --declaration ./src/index.ts && npm run postbuild"
timiscoding commented 3 years ago

I've created a npm fork @timiscoding/react-tabulator if anyone wants to test the fix. But ideally, you'll probably want to use patch-package so you don't have to update all your imports in the code base.

As you can see from the diff, the require('tabulator-tables') import is now applied to __importDefault() which falls back to creating an object with a default property to the module.

https://github.com/timiscoding/react-tabulator/commit/0decbd201e4433dde19313a6fc4e169c36dba305#diff-e6427b57cc4fd1d3909cd8ff56371e8a4e23b90dc66ab1995abb37d1990c076aR98

mike-lischke commented 3 years ago

@timiscoding Thanks for working this out! I'd prefer, however, that @ngduc takes this in to the original react-tabulator node module. Would make things easier on my end.

In any case, I'm happy to see there's a solution.

timiscoding commented 3 years ago

@mike-lischke Of course, the best way would be to update the original package. Hoping to get confirmation that it indeed fixes it for others.

Lost708 commented 3 years ago

@timiscoding - I downloaded your fork today to see if it fixed the issues we were having with React unit tests failing. Indeed it did! Would be GREAT if this was incorporated into an update in the original package. Thank you for addressing the issue.

mike-lischke commented 3 years ago

@timiscoding I also found the time finally to test your fork and also for me it worked. Many thanks!

However, I see a problem with mounting the underlying tabulator table in my componentDidMount and componentDidUpdate() methods (running from Jest tests). The table field of my ref.current is undefined in both cases, as if tabulator was never mounted.

I don't know if that's a specific problem of your fork or something that @ngduc should look at. Is it possible to "force" mounting the underlying tabulator when the ReactTabulator is mounted?

timiscoding commented 3 years ago

@mike-lischke Good to hear it! Are you using react-test-renderer? Refs don’t work because it doesn’t mimic a full dom environment. So you have to mock the refs using ‘create NodeMock’ api https://reactjs.org/docs/test-renderer.html#ideas

https://github.com/facebook/react/issues/7740

The only change I made in the fork was in the build step so apart from that, the code would be functionally identical to 0.15.0

mike-lischke commented 3 years ago

Thanks for the hint. Yes, I'm using react-test-render but also enzyme, where I can mount components. But right, that doesn't mean they also have a DOM. Looks like I need to see what I can do with createNodeMock or similar.

Doesn't belong to the discussion here, however. I hope your changes will be taken over to the main repo sooner than later. Have a good time!

timiscoding commented 3 years ago

I haven’t used enzyme in a while but could be related to incompatible adapters?

https://stackoverflow.com/a/55292957

mike-lischke commented 3 years ago

The current member is set in my tests. What's missing is the current.table, i.e. the underlying Tabulator instance is not yet created.

kakarot218 commented 1 year ago

@timiscoding is it safe to use your package?

timiscoding commented 1 year ago

The only change is adding a ts compiler option in the build script and running it. I haven't used the original package since my last comment so I'd try the latest release to see if it has already fixed this issue.

https://github.com/timiscoding/react-tabulator/commit/0decbd201e4433dde19313a6fc4e169c36dba305