Jsdom: Unexpected token in webidl-conversions

[su30007]

Basic info:

• Node.js version: 10
• jsdom version: 16.22

Minimal reproduction case

import React from 'react';
import jsdom from 'jsdom';
import { Header, Footer, LinksContainer, Toolbox } from '../components';
import App from '../App';

const { JSDOM } = jsdom;
const { window } = new JSDOM(`...`);
describe('Landing page', () => {
  beforeEach(() => {
    jest.resetModules();
  });

  afterEach(() => {
    jest.clearAllMocks();
    jest.clearAllTimers();
  });

  it('should render without throwing an error', () => {
    window.chrome = {
      runtime: {}
    };

    const wrapper = shallow(<App />);

    expect(wrapper.find('h1').text()).toEqual(i18n('LANDING_PAGE_TITLE'));
    expect(wrapper.find(Header)).toBeDefined();
    expect(wrapper.find(Toolbox)).toBeDefined();
    expect(wrapper.find(LinksContainer).length).toEqual(2);
    expect(wrapper.find(Footer)).toBeDefined();

    window.chrome.webstore = {};
    wrapper.setProps({ test: 'test' });
  });

  it('handleCancel should hide the modal', () => {
    window.chrome = false;
    const wrapper = shallow(<App />);
    const instance = wrapper.instance();

    instance.handleCancel();

    expect(wrapper.state('visible')).toEqual(false);
  });
});

... your code that reproduces the problem here, probably using dom.window ...

How does similar code behave in browsers?

Run a unit test by jest, then you will see

● Test suite failed to run

/node_modules/jsdom/node_modules/webidl-conversions/lib/index.js: Unexpecte
d token, expected ( (357:12)

    355 | 
    356 |         return true;
  > 357 |     } catch {
        |             ^
    358 |         return false;
    359 |     }
    360 | }

[domenic]

This does not follow the issue template, as it includes technologies besides jsdom (jest, react) and uses syntax (import, JSX) that is not supported in Node.js v10. Please add a comment after you fix the code sample to follow the issue template and verify that your new code sample still causes an issue.

[partyka1]

I have this issue just from importing jsdom in new empty project, node 8.17.0 :(

[eleanombre]

@partyka1 I just had this issue, it disappeared when using Node v10.16.1 (npm v6.9.0).

[Tsenzuk]

The issue is reproducible on node v8

[erickmiller]

The problem is that one of the dependencies webidl-conversions has a try / catch block that is not passing the identifier into the catch statement which is not following the javascript es5 language standard (despite some support for this incorrect syntax across browsers, this is incorrect javascript code). http://es5.github.io/#x12.14 I'm going to take a look at webidl-conversions right now and see about patching it or if there is a newer version that does not have this incorrect javascript code.

[erickmiller]

The problematic code that's causing webpack to fail is on line 362 of webidl-conversions which I didn't realize is a jsdom project:
https://github.com/jsdom/webidl-conversions/blob/dd42044efbf4844c630270ce0d6b04a2a3d14a75/lib/index.js
I'll try patching it locally and see if the rest of it will compile. Will report back.

[erickmiller]

This same issue catch without an identifier getting passed exists in:
jsdom/lib/jsdom/living/helpers/dates-and-times.js
jsdom/lib/jsdom/living/custom-elements/CustomElementRegistry-impl.js

I patched these files locally changing catch { to catch (e) in a few places in these files and it's fixed all the compilation errors. I'll fork and submit a pull request in a minute once I test a bit more locally.

[erickmiller]

Ok simple fixes but this got the project compiling again, went ahead and submitted pull requests here:
jsdom:
https://github.com/jsdom/jsdom/pull/3094
webidl-conversions:
https://github.com/jsdom/webidl-conversions/pull/35
Cheers.

[domenic]

Node v8 is not supported, as noted in one of the very first lines of the README.

[erickmiller]

Domenic, your assumption about Node compatibility is incorrect. We are NOT running node v8 and we are running a node version far past version 10 -- this has nothing to do with the Node version -- this is an an incompatibility with the javascript standard and apparently an incompatibility with https://github.com/webpack/webpack and it makes no sense to not fix these few lines of incorrect javascript syntax in your code. Why close this simple and helpful pull request without merging? 99% of the places in the repo call try / catch correctly. There are 3 files that contain a syntactically incorrect call to the catch statement. I fixed it, took me 5 minutes. This is a very useful library but is no good if it can't compile with other industry standard javscript tools. You should accept the change. Why would you not want to? Makes no sense. Will have to keep patching on each build. Explain any logical reason to keep incorrect syntax in these few files when the fix is so incredibly simple and makes no difference to the logical runtime of the code.

[erickmiller]

Since these changes have not been accepted, leaving jsdom not able to compile using the latest state of the art javascript tools and stack on node -- feel free to use my forked versions until this project decides to comply with the javascript standard. Note: I will not be maintaining these libraries so this is only a temporary fix until jsdom fixes the incorrect catch statement syntax.
https://github.com/erickmiller/jsdom
https://github.com/erickmiller/webidl-conversions

[domenic]

Binding-less catch is part of the JavaScript standard. https://tc39.es/ecma262/#prod-Catch

[Tsenzuk]

Yes, the syntax is a part of our standard, but I think it's a good idea to omit to break backward compatibility if it's possible. There are still a lot of environments in production that can't be updated to node v10 because of different conditions.

E.g.: in my case, some of the productions are running in closed environments without internet and it's hard to pass a new node there, but some fixes should be done for them also.
To be on the same page, I need to use the same archaic node.js, but build and test tools that are dependent on jsdom downloads it's a new version and stops working.

[domenic]

We're not interested in supporting old environments in our project.

[erickmiller]

Ok yes I agree try / catch re: es6 standard, you are correct. But I can assure you our environment is very up to date. We are not running some old version of node and most of our codebase is es6 and has been for a long time. From what I can see multiple other users in this community have also posted that they are running node v10, yet they are all still experiencing this compilation error. This is not just an "old version of node" issue as you've assumed, or some old environment issue. This bug is causing Webpack to fail which has something like 6 million users. I would 100% understand not accepting the pull requests for these simple fixes, if fixing this could cause some other feature to break, or jeopardize some other better features of the project. But that is clearly not the case in this instance, and it's not like we are suddenly opening the doors to full es5 backwards compatibility. You are talking with a group of people who are all running node v10 or greater yet are all experiencing this issue. It would be really cool of you to be reasonable and merge this fix so that at some later date the patches we're now deploying don't cause some other unforeseeable bug and/or maintenance issues across a broad selection of your user base that are using modern javascript tools that currently can't compile without these simple fixes. So perhaps just merge these simple fully compatible and easy changes, right? Isn't that the responsible and rational action to take in this particular case?

[domenic]

If Webpack does not support modern JavaScript, then we're not interested in supporting Webpack.

[erickmiller]

We're not interested in supporting old environments in our project.

Also, if you did truly just want to make the codebase incompatible with older versions of node, or older javascript standards, then there are more explicit and direct ways of doing so. If this is something that the project truly cares about on an engineering level. Yes? Such as perhaps https://nodejs.org/docs/v0.10.12/api/process.html#process_process_version Then check and throw an error explicitly stating that this version of node isn't supported, etc. Or this version of javascript isn't supported. From what I've been able to tell, this particular fix is not related to "old" environments despite continuously stating such. I haven't tracked down which part of webpack throws this error but thats where the build fails on jsdom in this case.

[erickmiller]

If Webpack does not support modern JavaScript, then we're not interested in supporting Webpack.

Ok sure, that's a fair point, but from what I can tell the issue is more nuanced that this. We are on a fairly recent version of webpack. Not sure if you're familiar with webpack but it's a pervasive bundler used by a ton of people/companies/projects etc. Webpack obviously and certainly 100% supports modern JavaScript we're using es6 syntax all over the place, never with an issue. For some reason it's this one catch statement throws, sure perhaps something overlooked in the compile somewhere in the webpack build, but I can assure you fixing this issue in jsdom isn't creating a doorway where everyone will suddenly expect you to maintain es5 backwards compatibility. The fix is so simple, also, it truly would be helpful to your commmunity and everyone using the jsdom project. Are you interested in that?

[domenic]

No. I really urge you to file an issue on webpack.

We're writing standards-compliant code; if other tools can't deal with that, it's not our bug, and we don't intend to take on the maintenance burden of staying compatible with whatever non-standards-compliant tools people are combining our library with.

[erickmiller]

Ok sure, will look into this further re:webpack when I have more time. I understand your position but this is a very odd thing to not just allow the merge -- there is no burden created here. It was something like 5 lines of 100% compatible, automatically mergeable code. And it would fix compilation issues for an unknown-many number of this project's users, and it would lower your burden so that you don't have to deal with this issue as it will undoubtably continue to arise considering the various stacks users are running that are atop of node. Literally have a project with hundreds of node packages and this is the only one that throws on compilation. Defending this as a "modern" coding standards decision is odd. Not explicitly checking for the version you are compatible with and throwing an appropriate error makes it clear that the "modern" standards excuse is more of an afterthought, rather than a clearly engineered and intentional decision. Not accepting this fix is irrational, but you are maintaining it so that's your decision. I chose this project thinking it was going to be the best most well maintained dom parser. The more likely route rather than opening issues with webpack which would require changing bundler versions first will be to switch the dom parser to something like https://github.com/fb55/htmlparser2 which has about 2M more users than this project. Unfortunate, though. Really.