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

[Feature] Update ReasonReact JSX to version 3 #1279

Merged
merged 15 commits into from May 28, 2019

Conversation

@arecvlohe
Copy link
Contributor

commented Apr 30, 2019

Summary

This PR updates the ReasonReact jsx syntax to version 3, bsb-platform to 5.0.4, bs-react-helmet to 2.0.0, and reason-react to 0.7.0. In addition, the example reason-react-app is now updated to use jsx version 3.

arecvlohe added 2 commits Apr 30, 2019
yarn.lock Outdated Show resolved Hide resolved
@arecvlohe
Copy link
Contributor Author

left a comment

Thoughts?

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@MoOx

This comment has been minimized.

Copy link
Member

commented May 1, 2019

To be sure the thing is working, you also need to edit example/reason-react-app.
That's where you can specify an actual reason-react version & run a build :)

Good start anyway!

@arecvlohe

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Cool! I will try and work on this more tonight, but might be tomorrow. Also, I realized that we might have to use ReasonReactCompat.wrapReasonReactForReact in cases where we can't convert jsx 2 to jsx 3.

@arecvlohe

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@MoOx Still just working along and making necessary changes but running into an issue that I am not able to diagnose:

sh: /Users/arecvlohe/Documents/development/phenomic/node_modules/bs-platform/lib/bsppx.exe -bs-jsx 3: No such file or directory
File "", line 1:
Error: Error while running external preprocessor
Command line: '/Users/arecvlohe/Documents/development/phenomic/node_modules/bs-platform/lib/bsppx.exe -bs-jsx 3' '/var/folders/h6/x5vgndfs0dv_y2017t23x51m0000gn/T/camlppx3bf7df' '/var/folders/h6/x5vgndfs0dv_y2017t23x51m0000gn/T/camlppxf075b9'

The error will show in your editor at the very top of link.re. It seems like everything else builds fine. I was having this issue in other files but it's only an issue in one now. I will take a look at this some more tomorrow night but wanted to leave it here in case you could take a look at it.

@MoOx
Copy link
Member

left a comment

The error your are facing is related to this jaredly/reason-language-server#275

I am trying to fix the vscode plugin (I assume you are getting it from there) but it make take time until fix is released.
You can try to run yarn run test at the root (can take a few minutes, but run all tests, including bs compilation)

@arecvlohe

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

@MoOx Thanks for the context, it looks like the error is coming from RLS. I still have errors when running yarn run test so I will look into that more today.

@arecvlohe

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

@MoOx Leaving a comment here because I can't figure this out. When I run yarn run transpile I am getting a syntax error in link.re on the line where it has React.element. I am not sure why this is happening. I Also found it strange that in Link.js no children are passed but when it's used a React.array is passed to <Link />. Maybe this is shows a lack of understanding but wanted to make note of it so I can learn something. Anyhow, the syntax error is main thing that is blocking me so any help would be appreciated!

UPDATE: Figured it out :)

@arecvlohe

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

@MoOx Sorry for spamming you on a Saturday but I think the code is now ready. After running yarn run test, all the tests pass. It looks as though there is a build error though, something related to using &&. You probably would know more about that than me. Anyhow, have a good weekend and let me know if the code needs any more tweaking!

@arecvlohe arecvlohe marked this pull request as ready for review May 4, 2019

@arecvlohe arecvlohe changed the title [Feature] Update to reason react jsx to version 3 [Feature] Update ReasonReact jsx to version 3 May 5, 2019

@arecvlohe arecvlohe changed the title [Feature] Update ReasonReact jsx to version 3 [Feature] Update ReasonReact JSX to version 3 May 5, 2019

@MoOx
Copy link
Member

left a comment

Good stuff. Just a single question.

I will release this as 2.0.0 as this is a breaking change for reasonml users.
Otherwise we could avoid the breaking change release & go for another plugin etc (but this will create a mess).

Phenomic is in a bad state atm (because not really maintained - I have ideas but busy with pure reasonml projects) and I might end up spltting this big project as smaller parts (under the phenomic org, cause same goal: do static website)

examples/reason-react-app/App.js Outdated Show resolved Hide resolved
@arecvlohe

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@MoOx Done. I can see the benefit of splitting it up into different packages. Might make it easier to maintain overall. If more people contribute/maintain then it would be easier for one person to maintain one library instead of many.

@MoOx
Copy link
Member

left a comment

I think we should change the signature of the hoc

@arecvlohe

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@MoOx Done. I thought this made more sense (React.component('a), Js.t({..})) => React.component('b) since it's a HOC, it should be going from 'a to 'b.

@arecvlohe

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@MoOx ping

1 similar comment
@arecvlohe

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@MoOx ping

@MoOx
MoOx approved these changes May 28, 2019
Copy link
Member

left a comment

Thanks, sorry for the lag

@MoOx MoOx merged commit b3f49c4 into phenomic:master May 28, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
deploy/netlify Deploy preview failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MoOx

This comment has been minimized.

Copy link
Member

commented May 28, 2019

1.0.0 is out on npm (just include this change). Thanks for helping!

@arecvlohe

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@MoOx Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.