#84 - add displayName for @:jsxStatic components #86

kLabz · 2017-12-04T23:24:33Z

When compiled with -debug, add a displayName to functional components for use with external tools (enzyme, chrome dev tools, etc.).

Handled components include:

Component classes with @:jsxStatic meta
Static methods returning a ReactElement
Static variables with getter returning a ReactElement

Local vars, static vars (without getter) & instance fields are not handled -- they will still be displayed as "Unknown". It's because they are not constant so it's a lot more complex.

Output example, at the end of the generated js file:

components_Footer.render.displayName = components_Footer.render.displayName || "Footer";

This should be compatible with haxe-modular code splitting, but this has not been tested yet (I need to setup a sample project).

Cleanup: moved all jsxStatic-related code to react.jsx.JsxStaticMacro.hx

elsassph · 2017-12-05T08:35:24Z

src/lib/react/ReactMacro.hx

+			name: '__displayName',
+			access: [Access.AStatic, Access.APrivate],
+			kind: FieldType.FVar(null, macro {
+				untyped $i{classPath}.$jsxStaticField


I'm afraid this won't be compatible with code splitting - what does the generated JS look like?

To work with Modular it should be added in static _init_ and the code should be a direct assignement:

com_Something.StaticField.displayName = 'StaticField';

Though we'd need to check but maybe this will work as well (in _init_):

if (com_Something.StaticField != null) com_Something.StaticField.displayName = 'StaticField';

This adds assignements alongside tagComponent() output, at the end of the generated js:

components_Footer.__displayName = components_Footer.render ? components_Footer.render.displayName = "Footer" : false; components_Link.displayName = "Link"; // From tagComponent() components_Todo.__displayName = components_Todo.render ? components_Todo.render.displayName = "Todo" : false;

I'm not sure about code splitting, but it seems to work (or not to) like existing code. I'd need to check that.

Btw, direct assignement without untyped (or similar tricks) will not be possible, haxe won't add fields to functions.

Ok that would be compatible with code splitting.

elsassph · 2017-12-05T08:43:13Z

src/lib/react/ReactMacro.hx

@@ -295,6 +295,37 @@ class ReactMacro
 		return fields;
 	}

+	static function buildJsxStatic():Array<Field> {


Could use a comment saying "Add a displayName to functional components"

elsassph · 2017-12-05T08:49:01Z

extraParams.hxml

@@ -0,0 +1 @@
+--macro addGlobalMetadata("", "@:build(react.ReactMacro.buildJsxStatic())")


This makes me sad.

And though it works I'm not sure this is the best approach: we'll have the same problem in other cases where we just use a functional component without using :jsxStatic:

function render() { return jsx('<FunctionalComp />'); } static function FunctionalComp(props) { ... }

I agree.
Maybe this is one of several things that could be added in jsx macro, which might need a (partial?) rewrite to handle more complex features.

I have a working prototype without extraParams, handled within jsx macro.
I'll clean things up a bit, factorize a little and try to find edge cases (local variables?).

The output will be a little different (but better imo), and still at the end of the generated code:

components_Footer.render.displayName = "Footer"; components_Todo.render.displayName = "Todo";

Edit:
Output will actually be like this, to allow user defined displayNames:

components_Footer.render.displayName = components_Footer.render.displayName || "Footer";

kLabz · 2017-12-06T17:31:38Z

I updated the PR, using jsx macro as entry point instead of adding build macros.

djaonourside · 2017-12-25T09:21:23Z

Hello, gentlemen! Any chance this pr will be accepted?

kLabz · 2017-12-29T14:32:12Z

This PR is ready imo, only waiting for @elsassph approval.
Could you please take a look when you have time to do so?

I had rewritten the PR description along with the feature itself, and your comments should have been handled.
The diff may seem heavier than it really is, because I moved some existing code to a dedicated class so that @:jsxStatic-related code do not polute JsxMacro.

elsassph · 2017-12-30T09:25:22Z

Does it work if @:jsxStatic is applied to a sub-class of a module? e.g.

// Foo.hx
class Foo {...}

@:jsxStatic
class Bar {}

kLabz · 2017-12-30T09:41:26Z

Yep.

I found a small issue with @:jsxStatic however: it breaks without giving a clear explanation (so ReactMacro breaks later) when used without any param. Should we send a proper compilation error, or default to some value? ('render', maybe?)

elsassph · 2017-12-30T10:44:58Z

A default value could help but it may cause other issues and confusion if you have a non-static method of the default name...

kLabz · 2017-12-30T11:31:08Z

The error would be the same if the field does not exist or if it is not a static method:

Class<X> has no field render

But an error is probably better indeed. Defaulting to "render" would save a few keystrokes but I agree that it is less safe. I'll add the compilation error message in this PR.

kLabz · 2017-12-30T12:11:46Z

Added the compilation error if no param.

elsassph · 2017-12-31T13:59:14Z

src/lib/react/jsx/JsxStaticMacro.hx

+			case WithParams(_, params): params.pop().getValue();
+			case NoParams(meta):
+				Context.fatalError(
+					"@:jsxStatic requires a param (name of the component's function)",


Maybe more like Parameter required for @:jsxStatic('name-of-static-function')

djaonourside · 2018-01-23T11:49:24Z

Hello, guys! Any chance these changes will be in haxelib version?

kLabz · 2018-02-05T16:20:49Z

Anything else bothering you @elsassph? (btw, do you want me to squash?)

elsassph · 2018-02-06T16:58:43Z

src/lib/react/jsx/JsxStaticMacro.hx

+		if (!hasBeenHooked)
+		{
+			hasBeenHooked = true;
+			Context.onAfterTyping(afterTypingHook);


Does that work with compiler server?

Can't find a way to break it, do you have something specific in mind?

$ haxe --connect 9009 build.hxml --times -debug /git/haxe-react/src/lib/react/jsx/JsxStaticMacro.hx:153: afterTyping /git/haxe-react/src/lib/react/jsx/JsxStaticMacro.hx:153: afterTyping name | time(s) | % | # | info ---------------------------------------- analyzer | 0.090 | 9 | 30285 | filters | 0.050 | 5 | 1 | generate | 0.010 | 1 | 1 | js | 0.010 | 1 | 1 | haxelib | 0.000 | 0 | 1 | macro | 0.550 | 56 | 361 | other | 0.080 | 8 | 1 | parsing | 0.120 | 12 | 647 | server | 0.010 | 1 | 8692 | module cache | 0.010 | 1 | 8692 | typing | 0.080 | 8 | 1116 | ---------------------------------------- total | 0.990 | 100 | 0 | $ cat .build/klabz.js | grep displayName | wc -l 65 $ haxe --connect 9009 build.hxml --times -debug name | time(s) | % | # | info ---------------------------------------- analyzer | 0.000 | 0 | 37 | filters | 0.020 | 40 | 1 | generate | 0.020 | 40 | 1 | js | 0.020 | 40 | 1 | haxelib | 0.000 | 0 | 1 | other | 0.000 | 0 | 1 | server | 0.010 | 20 | 649 | module cache | 0.010 | 20 | 649 | add modules | 0.010 | 20 | 7 | changed dirs | 0.000 | 0 | 628 | check | 0.000 | 0 | 7 | typing | 0.000 | 0 | 1 | ---------------------------------------- total | 0.050 | 100 | 0 | $ cat .build/klabz.js | grep displayName | wc -l 65 $ haxe build.hxml $ cat .build/klabz.js | grep displayName | wc -l 35 $ haxe build.hxml -debug /git/haxe-react/src/lib/react/jsx/JsxStaticMacro.hx:153: afterTyping /git/haxe-react/src/lib/react/jsx/JsxStaticMacro.hx:153: afterTyping $ cat .build/klabz.js | grep displayName | wc -l 65

elsassph reviewed Dec 5, 2017

View reviewed changes

kLabz force-pushed the feature/jsxstatic-displayname branch from 1d143df to cec106b Compare December 6, 2017 17:19

kLabz force-pushed the feature/jsxstatic-displayname branch from 018f1b9 to 98fad4a Compare December 7, 2017 16:41

elsassph reviewed Dec 31, 2017

View reviewed changes

kLabz force-pushed the feature/jsxstatic-displayname branch from ee020a3 to da00c23 Compare December 31, 2017 15:10

elsassph reviewed Feb 6, 2018

View reviewed changes

kLabz force-pushed the feature/jsxstatic-displayname branch from e5e3b42 to 1c6a346 Compare February 7, 2018 09:51

kLabz added 2 commits February 7, 2018 10:53

massive-oss#84 - add displayName for functional components

cf66714

jsxStatic macro cleanup

cfe497d

kLabz force-pushed the feature/jsxstatic-displayname branch from 1c6a346 to 7e46690 Compare February 7, 2018 09:58

kLabz mentioned this pull request Feb 25, 2018

Higher Order Components (HOC) elsassph/haxe-redux#6

Closed

@:jsxStatic: add compilation error if no param

dc9ab98

kLabz force-pushed the feature/jsxstatic-displayname branch from 7e46690 to dc9ab98 Compare February 26, 2018 09:33

elsassph merged commit 9755a24 into massive-oss:master Feb 27, 2018

elsassph mentioned this pull request May 3, 2018

Functional components is shown as <Unknow>...</Unknow> in chrome dev tools #84

Closed

kLabz deleted the feature/jsxstatic-displayname branch January 17, 2019 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#84 - add displayName for @:jsxStatic components #86

#84 - add displayName for @:jsxStatic components #86

kLabz commented Dec 4, 2017 •

edited

Loading

elsassph Dec 5, 2017

kLabz Dec 5, 2017 •

edited

Loading

elsassph Dec 5, 2017

elsassph Dec 5, 2017

kLabz Dec 5, 2017

elsassph Dec 5, 2017

kLabz Dec 5, 2017

kLabz Dec 6, 2017 •

edited

Loading

kLabz commented Dec 6, 2017

djaonourside commented Dec 25, 2017

kLabz commented Dec 29, 2017

elsassph commented Dec 30, 2017

kLabz commented Dec 30, 2017

elsassph commented Dec 30, 2017

kLabz commented Dec 30, 2017 •

edited

Loading

kLabz commented Dec 30, 2017

elsassph Dec 31, 2017

djaonourside commented Jan 23, 2018

kLabz commented Feb 5, 2018

elsassph Feb 6, 2018

kLabz Feb 7, 2018

		@@ -0,0 +1 @@
		--macro addGlobalMetadata("", "@:build(react.ReactMacro.buildJsxStatic())")

#84 - add displayName for @:jsxStatic components #86

#84 - add displayName for @:jsxStatic components #86

Conversation

kLabz commented Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

kLabz Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kLabz Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

kLabz commented Dec 6, 2017

djaonourside commented Dec 25, 2017

kLabz commented Dec 29, 2017

elsassph commented Dec 30, 2017

kLabz commented Dec 30, 2017

elsassph commented Dec 30, 2017

kLabz commented Dec 30, 2017 • edited Loading

kLabz commented Dec 30, 2017

Choose a reason for hiding this comment

djaonourside commented Jan 23, 2018

kLabz commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kLabz commented Dec 4, 2017 •

edited

Loading

kLabz Dec 5, 2017 •

edited

Loading

kLabz Dec 6, 2017 •

edited

Loading

kLabz commented Dec 30, 2017 •

edited

Loading