faulty assertion for React component with React.Fragment causes concordance to crash

[iamstarkov]

Description

Test Source

This one works correctly

import React from 'react';
import { shallow } from 'enzyme';
import test from 'ava';

const ConcordanceBugDemoOkay = () => <div />;
test('demo okay', t => {
  const wrapper = shallow(<ConcordanceBugDemoOkay />);
  t.falsy(wrapper);
});

this one does not work correctly:

import React from 'react';
import { shallow } from 'enzyme';
import test from 'ava';

const ConcordanceBugDemoNotOkay = () => <React.Fragment />;
test('demo', t => {
  const wrapper = shallow(<ConcordanceBugDemoNotOkay />);
  t.falsy(wrapper);
});

and causes concordance to crash because it tries to concatenate string with Symbol.

Error Message & Stack Trace

/Users/iamstarkov/projects/project/node_modules/concordance/lib/formatUtils.js:6

Error thrown in test:

TypeError {
  message: 'Cannot convert a Symbol value to a string',
}

Object.wrap [as wrapFromTheme] (node_modules/concordance/lib/formatUtils.js:6:25)
Object.formatName (node_modules/@concordance/react/lib/elementFactory.js:109:29)
Object.finalize (node_modules/@concordance/react/lib/elementFactory.js:133:29)
formatDescriptor (node_modules/concordance/lib/format.js:85:42)
Object.format (node_modules/concordance/lib/format.js:99:10)

Config

N/A

Command-Line Arguments

N/A

Environment

Node.js v8.12.0
darwin 17.7.0
ava --version # 1.0.0-beta.8
npm --version # 6.4.1
yarn --version # 1.9.4

[novemberborn]

Which React version is this?

[iamstarkov]

I think I have a fix

[novemberborn]

Looks like the name is now a symbol, rather than a string. Should be fixable in @concordance/react.

[iamstarkov]

Yes, it is a Symbol now. We fixed it by fixing this line https://github.com/concordancejs/concordance/blob/master/lib/formatUtils.js#L6:

-  return fromTheme.open + value + fromTheme.close
+  return fromTheme.open + value.toString() + fromTheme.close

[iamstarkov]

is it good enough fix?

[iamstarkov]

it will work for all versions of react with Fragment

[novemberborn]

We should fix this at the call site: https://github.com/concordancejs/react/blob/7f8101f6f67fd03249af28828b16e69e9d914b60/lib/elementFactory.js#L109

Assuming the symbol is registered, we could detect it and then use React.Fragment as the name. But there should be a fallback for other symbols.

[iamstarkov]

well, Im sorry. you lost me on registered symbol.

But I still think it will be a good idea to defend lib/formatUtils.js@wrap against non string instances. What do you think?

[novemberborn]

well, Im sorry. you lost me on registered symbol.

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/for.

But I still think it will be a good idea to defend lib/formatUtils.js@wrap against non string instances. What do you think?

No, consider it typed as taking value as a string. The problem is in @concordance/react. I'd been meaning to take a look at that. There's a flurry of Concordance activity all of a sudden! 😀

[iamstarkov]

I looked into it for quite some time and cant figure out how to do it =(

[novemberborn]

This should be fixed with concordancejs/react#14.

I have one more PR that needs to land in concordance itself, and then I need to do the upgrade dance to get these changes into AVA itself.