Better es6 class support

[dobesv]

Use Reflect to construct class in a more compatible manner.

Reflect should be available in environments that use es6 classes.

This can fix #999 because we preserve the same value of this as was used in the object constructor, whereas the previous implementation discarded that object and used a new object after calling the constructor. That resulted in some confusing behavior if the constructor stored referenced to this or bound methods / functions to this.

[coveralls]

Coverage increased (+0.9%) to 94.892% when pulling 0f7521f on dobesv:master into 55a6965 on FormidableLabs:master.

[ryan-roemer]

Thanks for opening this!!!

In case it helps as we're getting ci fixed up on this, probably about time to switch Travis versions to:

node_js:
  - "6"
  - "8"
  - "10"

(Appveyor is already there...)

[ryan-roemer]

Something we've added is failing on uglify

ERROR in radium.min.js from UglifyJs
Unexpected token: punc (.) [radium.min.js:508,11]
[builder:proc:end:2] Command: webpack --config=webpack.config.minified.js
[builder:proc:error] Code: 2, Command: webpack --config=webpack.config.minified.js
[builder:builder-core:end:5230] Task: concurrent build-dist-dev, build-dist-prod, Error: Command failed: sh -c webpack --config=webpack.config.minified.js
[builder:proc:end:2] Command: builder concurrent --buffer build-dist-dev build-dist-prod
[builder:proc:error] Code: 2, Command: builder concurrent --buffer build-dist-dev build-dist-prod
[builder:builder-core:end:5215] Task: concurrent build-lib, build-dist, build-es, Error: Command failed: sh -c builder concurrent --buffer build-dist-dev build-dist-prod
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Probably worth looking at transpiled output (webpack should be consuming es/* files). If it's an ES6 thing that should be supported, we can look to updating the webpack build to use something more es-friendly...

[ryan-roemer]

And to save you round trips to CI, all of this should work/fail the same in localhost:

$ yarn run build-examples
$ yarn run lint
$ yarn run build
$ yarn run test-coverage

[dobesv]

Yeah, I'm working on the CI fixes. I think I don't need to use new.target, which is what uglifyjs is mad about. this.constructor should always be available and correct from my understanding.

[dobesv]

Looks like tests are passing. Not sure if you have any other way to verify this won't break things for people who were using es native classes out there. It does seem to fix the issue for me if I hand-patch node_modules using the generated file from lib.

[ryan-roemer]

@dobesv -- Great work! You changes still pass the original regression test I wrote for ES native classes at:

radium/test/enhancer-test.js

Lines 56 to 66 in 0f7521f

	// Regression test - `Radium wrapping not compatible with native classes?`
	// https://github.com/FormidableLabs/radium/issues/576
	it('handles native ES classes', () => {
	class Composed extends React.Component {
	render() {
	return testElement;
	}
	}
	assertValidEnhancedComponent(Composed);
	});

so I think we should be good.

@boygirl @alexlande -- We've got some great work here that I don't have time right now to kick the tires on. Can you recruit someone to vet this and possibly merge + release? Thanks!!!

[dobesv]

Any ETA on this one?

[ryan-roemer]

I believe @kylecesmat is going to look at this when he has a chance, hopefully this upcoming week. Thanks for your patience!

(I'd jump in and try to wrangle it, but I'm on parental leave with a newborn 👶 )

[kylecesmat]

I'm checking this out right now!

[kylecesmat]

I do want to see if we can refactor some of our existing tests to use jsdom/enzyme - in which case moving these to top-level requires. But this implementation is great for now.

[dobesv]

Yeah, since I'm not a core contributor I wanted to have minimal impact.

[kylecesmat]

Thanks for updating this!

[dobesv]

Well, I had to in order to get a green light on the PR :)

[kylecesmat]

What are the implications of using Reflect in regards to cross-browser support? I believe IE11 is the only browser that will need a polyfill, would you be able to add a note in the README about including a polyfill like https://github.com/zloirock/core-js#ecmascript-reflect?

A note in this section would probably be best - https://github.com/FormidableLabs/radium#importing-radium

[dobesv]

@kylecesmat I don't think you can create a "native class" in IE11, so this code would never run in that browser.

[kylecesmat]

Apologies for the delay, your PR looks great! Just one ask to make a note about requiring a polyfill for Reflect in IE, and after that I'll merge + release. I'm actually going to take care of this before release.

I believe this should be released as a patch. A minor release feels a bit safer, so that IE11 consumers are not inheriting these changes.

[kylecesmat]

This has been released in radium@0.25.0