GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — `@:jsxStatic`: add field for identifying jsxStatic components by kLabz · Pull Request #107 · massive-oss/haxe-react

Conversation

@kLabz

@kLabz kLabz commented May 19, 2018

Copy link
Copy Markdown
Contributor

Allows identification of a component as a @:jsxStatic component outside the jsx(' ... ') macro, by adding a __jsxStatic:CreateElementType static field to classes with @:jsxStatic meta.

Example usage:

private abstract BundledComponent(Class<ReactComponent>) {
	// ...

	@:to
	public function toCreateElementType():CreateElementType {
		if (untyped this.__jsxStatic != null)
			return untyped this.__jsxStatic;

		return this;
	}
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kLabz
@:jsxStatic: add field for identifying jsxStatic components outside j…
1bbad11 
…sx macro
@elsassph

elsassph commented May 19, 2018

Copy link
Copy Markdown
Contributor

Why the global build macro?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kLabz

kLabz commented May 19, 2018

Copy link
Copy Markdown
Contributor Author

Because I have no other way to add a build macro to a class with @:jsxStatic meta, since this is not a ReactComponent class.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@elsassph

elsassph commented May 19, 2018

Copy link
Copy Markdown
Contributor

Isn't @:jsxStatic supposed to be used only on React classes normally?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kLabz

kLabz commented May 19, 2018

Copy link
Copy Markdown
Contributor Author

No, it's supposed to be used on non-React classes to make them work like React classes in the jsx context (but without the component lifecycle).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@elsassph

elsassph commented May 19, 2018

Copy link
Copy Markdown
Contributor

Ah... you want to solve cases where the class is never used with JSX... eh I don't like that :( but I guess there's no clean solution for that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
elsassph
elsassph reviewed May 19, 2018
View reviewed changes
Comment thread src/lib/react/jsx/JsxStaticMacro.hx Outdated

static public function build():Array<Field>
{
var fields = Context.getBuildFields();

@elsassph elsassph May 19, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can avoid calling getBuildFields and return null if we don't need to modify the fields. What would Tink do? ;)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@kLabz kLabz May 20, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always forget about this :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@kLabz kLabz May 20, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now the cost is almost null, it takes 0.015s on my 1.4s build.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
elsassph
elsassph reviewed May 19, 2018
View reviewed changes
Comment thread src/lib/react/jsx/JsxStaticMacro.hx Outdated
{
var fields = Context.getBuildFields();
var cls = Context.getLocalClass();
if (cls == null) return fields;

@elsassph elsassph May 19, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it really be null?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@kLabz kLabz May 20, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's a build macro called for every type, yes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kLabz

kLabz commented May 20, 2018

Copy link
Copy Markdown
Contributor Author

you want to solve cases where the class is never used with JSX

Not exactly. It doesn't change anything if the class has been used or not with jsx, it's handled there but not modified. I want to solve cases where the class is used outside JSX, whether or not it is used with JSX elsewhere in the project.

This could even (not 100% about that) be built into CreateElementType if we change it into an abstract, so that @:jsxStatic is handled everywhere we use CreateElementType (React.createElement(), HOC functions, etc.).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kLabz
Optimize jsxStatic build macro
fa2a335
@elsassph
elsassph merged commit a007f62 into massive-oss:master May 20, 2018
@kLabz
kLabz deleted the feature/jsxStatic-field branch January 17, 2019 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@elsassph elsassph elsassph left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@kLabz @elsassph
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.