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

Support hhvm 4.73 and xhp 4 #9

Merged
merged 16 commits into from
May 27, 2021
Merged

Support hhvm 4.73 and xhp 4 #9

merged 16 commits into from
May 27, 2021

Conversation

lexidor
Copy link
Contributor

@lexidor lexidor commented Nov 15, 2020

This PR makes the required changes to pass the typechecker with hhvm 4.73.
I have not updated the outdated React.createElement() javascript.
I have not formatted the code with a modern hackfmt, to keep the diff clear.
We can do a formatting PR later.

refs #6

It was a holder for functions and didn't need to be a class.
This code was written in a time when autoloading functions
was something that the default autoloader (composer) couldn't do.
I moved these pseudo contructors the member functions.
to_js_value (XHPJS::MapArgument) doesn't look like a public API,
so I movesed that to _Private.
@fredemmott
Copy link
Contributor

Sorry for not seeing this sooner; this is a great starting point, but for now I'd rather keep it complete than remove the react support, which is the main purpose for this project. @jjergus, do you agree?

@jjergus
Copy link

jjergus commented Jan 4, 2021

I was only vaguely aware of the existence of this project so don't have much context, but it seems to me that supporting non-React JS projects is still valuable?

Obviously it would be best to support both React and recent HHVM versions (xhp-lib 4), but if I only had to pick one I'd probably say support for recent HHVM/xhp-lib 4 is more important (but again, I don't really have much context).

@lexidor
Copy link
Contributor Author

lexidor commented Feb 24, 2021

React support was not removed. I merely marked it as deprecated to inform everyone that the current implementation does not work. I created opened #8.

Copy link

@jjergus jjergus left a comment

Choose a reason for hiding this comment

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

Looks good to me except for this one thing

src/x_js-scope.php Outdated Show resolved Hide resolved
@jjergus jjergus merged commit 05ce5bf into hhvm:master May 27, 2021
@jjergus
Copy link

jjergus commented May 27, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants