Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor changes for React 16 #80

Merged
merged 6 commits into from Nov 10, 2017
Merged

Conversation

@jasononeil
Copy link
Contributor

@jasononeil jasononeil commented Sep 30, 2017

  1. Add extern method for React.hydrate()
  2. Include ref and key values in inline objects (previously were not included if undefined, now included as null). Essentially removing the -D react_monomorphic option. In React 16, if ref is undefined rather than null, you get a runtime error. See facebook/react#10649

Fixes issue #78

jasononeil added 2 commits Sep 30, 2017
With React 16 "ref" must be null, not undefined, or you get errors while rendering:

> Error: Expected ref to be a function or a string.

See facebook/react#10649
@jasononeil
Copy link
Contributor Author

@jasononeil jasononeil commented Sep 30, 2017

Note: there could be further changes. The release is mostly backward compatible but there are some new things I haven't included yet that are probably worth including, here's a checklist:

@elsassph
Copy link
Member

@elsassph elsassph commented Sep 30, 2017

Nice, we also need to modify PropTypes for React 16.
Do you want to continue in this PR or stop here?

@kLabz
Copy link
Contributor

@kLabz kLabz commented Sep 30, 2017

Nice!

There's also ReactDOM.hydrate()

jasononeil added 2 commits Oct 1, 2017
Note: the documentation link points to the blog post, as this isn't included in the official React API docs yet (even though the React API docs seem to have been updated at the same time as the release?)
@jasononeil
Copy link
Contributor Author

@jasononeil jasononeil commented Oct 1, 2017

@kLabz ReactDOM.hydrate() is already in the PR :)

@elsassph for PropTypes - is that just a case of changing @:jsRequire('react', 'PropTypes') to @:jsRequire('prop-types')?

I was wondering if this needs a compiler define for switching between versions, but I don't think so - using require('prop-types') should be just fine on older versions of React (though might result in some code duplication if you're including proptypes in your runtime build I guess)

I thought about doing conditional compilation with a define to use the old import, but the new import should be compatible with old versions of React so I think I'll leave it.
@jasononeil
Copy link
Contributor Author

@jasononeil jasononeil commented Oct 1, 2017

@elsassph regarding render being able to return a String or Fragment (array of JSX nodes)... I think this has more to do with the JSX parsing macro than the types. The macro should now accept those types, and spit out a ReactElement at the end, correct?

In terms of whether to merge now or later, I'm in favour of merging this set of changes now, and working on any JSX adaptations needed as a separate PR. Whether to do a haxelib release or not is up to you :)

@jasononeil
Copy link
Contributor Author

@jasononeil jasononeil commented Oct 1, 2017

p.s. Sorry for the whitespace noise on ReactMacro.hx. Visual Studio Code did that automatically and it's GIT diff viewer doesn't show whitespace changes so I didn't notice when I committed.

I would love to get an automatic code-formatter for haxe to avoid noise like this :)

@kLabz
Copy link
Contributor

@kLabz kLabz commented Oct 1, 2017

Oh sorry, I should have re-read before commenting :P

@elsassph
Copy link
Member

@elsassph elsassph commented Oct 1, 2017

For returning strings/arrays it should just be allowed as return value for render. Though I wonder if it can be used anywhere a ReactElement is allowed.

@kLabz
Copy link
Contributor

@kLabz kLabz commented Oct 1, 2017

I'm doing some tests with function render():EitherType<ReactElement, EitherType<Array<ReactElement>, EitherType<String, Float>>>; (numbers are supported too), but it's not enough:

  • ReactDOM.render() (and hydrate I suppose) also supports these.
  • It's not really Array<ReactElement> that we need; mixed arrays with ReactElement / String / Numbers work (even nested arrays).

It seems to be available in most places where ReactElement is needed. It works for React.Children.map() for example, as both input and output. It won't work for React.cloneElement(), though.

Maybe we need an intermediate typedef which we'd use where we know it is safe to use any of these types, instead of modifying ReactElement.

Something like

typedef ReactFragment = EitherType<
	ReactElement,
	EitherType<
		Array<ReactFragment>,
		EitherType<String, Float>
	>
>;

Would work, except that we'd have to explicitly type our mixed arrays as Array<ReactFragment>, otherwise the haxe compiler would complain (or is there another trick for that?).

@elsassph
Copy link
Member

@elsassph elsassph commented Oct 1, 2017

Ew... At this point we may as well make it Dynamic :/

@jasononeil
Copy link
Contributor Author

@jasononeil jasononeil commented Oct 1, 2017

I really wish Haxe had a clean type for doing many EitherType joins.

In flow (which React itself uses) it would be something like render(): ReactElement | string | Array<ReactElement>, which is much nicer.

I think confining the ugliness to a new type like:

typedef ReactFragment = Either<ReactElement, String, Array<ReactElement>>

or more realistically

typedef ReactFragment = EitherType<ReactElement, EitherType<String, Array<ReactElement>>>

is probably best for now.

@jasononeil
Copy link
Contributor Author

@jasononeil jasononeil commented Oct 1, 2017

Just realised that's exactly what @kLabz suggested. Sorry for not reading properly!

@@ -3,7 +3,7 @@ package react;
/**
https://facebook.github.io/react/docs/top-level-api.html#react.proptypes
**/
@:jsRequire('react', 'PropTypes')
@:jsRequire('prop-types')

This comment has been minimized.

@elsassph

elsassph Oct 2, 2017
Member

To make it right it should honor the -D react_global flag:

#if (!react_global)
@:jsRequire("prop-types")
#end
@:native('PropTypes')
@elsassph
Copy link
Member

@elsassph elsassph commented Oct 4, 2017

@jasononeil sorry another PR just merged introduced some conflicts - the good news is that the PropTypes change is done properly.

@jasononeil
Copy link
Contributor Author

@jasononeil jasononeil commented Oct 4, 2017

Thanks @elsassph I'll resolve them and take a look. Sorry to have been a bit slow the last few days!

@jasononeil
Copy link
Contributor Author

@jasononeil jasononeil commented Oct 4, 2017

Merge conflicts resolved @elsassph. Nice work @kLabz with the improved PropTypes definitions!

@elsassph elsassph merged commit db912d2 into massiveinteractive:master Nov 10, 2017
@kLabz kLabz mentioned this pull request Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants