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

with-reasonml: use default exports #4217

Merged
merged 1 commit into from
Apr 27, 2018

Conversation

rase-
Copy link
Contributor

@rase- rase- commented Apr 27, 2018

Here is a suggestion for the ReasonML example to remove the js files for pages, which basically just wrap a default export.

ReasonML enables ES6 default exports so that's done here, but this way we have to use the lib/js directory the bucklescript compiler targets. I think it's a very acceptable tradeoff in this example.

@OlliV OlliV requested a review from timneutkens April 27, 2018 03:23
"dev": "concurrently \"bsb -clean-world -make-world -w\" \"next -w\"",
"build": "bsb -clean-world -make-world && next build",
"start": "bsb -clean-world -make-world && next start -w"
"dev": "bsb -clean-world -make-world && next dev lib/js",
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should actually use https://github.com/reasonml-community/bs-loader 🤔

Copy link
Contributor Author

@rase- rase- Apr 27, 2018

Choose a reason for hiding this comment

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

Probably could. I'm experimenting with that separately right now actually.

Don't think it's necessary for this PR tho. Already a nice incremental improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not bs-loader exactly, since it's deprecated, but a similarly smoother set up here could work, that is.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah definitely 👍

@timneutkens
Copy link
Member

Looks good to me

@timneutkens timneutkens merged commit f378448 into vercel:canary Apr 27, 2018
@rase- rase- deleted the feat/with-reasonml-default-exports branch April 27, 2018 03:36
@tmepple
Copy link

tmepple commented Apr 28, 2018

Hi @rase- Thanks for working on improving the with-reasonml example over the past few days. Some good stuff!

Quick question... with the new pattern from this PR are you able to call getInitialProps from ReasonML pages? I was using the old pattern in the .js file like this:

import { jsComponent as About } from "./About.re";

About.getInitialProps = async (context) => {
  // code here
};

export default () => <About />		

It is good to be able to avoid the extra .js file if possible with your change but not sure how to call getInitialProps in that case.

@rase-
Copy link
Contributor Author

rase- commented Apr 29, 2018

@tmepple Sure, you just have to do a little bit of hackery, similarly to how you'd have to do it with the separate JS file. Here's a quick example of how you could go about it (as a diff to the About page):

diff --git a/examples/with-reasonml/pages/about.re b/examples/with-reasonml/pages/about.re
index f104565..27c4db4 100644
--- a/examples/with-reasonml/pages/about.re
+++ b/examples/with-reasonml/pages/about.re
@@ -1,13 +1,24 @@
 let component = ReasonReact.statelessComponent("About");
 
-let make = (_children) => {
+let make = (~a, ~b, _children) => {
   ...component,
   render: (_self) =>
     <div>
       <Header />
       <p> (ReasonReact.stringToElement("This is the about page.")) </p>
+      <p> (ReasonReact.stringToElement(a)) </p>
+      <p> (ReasonReact.stringToElement(b)) </p>
       <Counter />
     </div>
 };
 
-let default = ReasonReact.wrapReasonForJs(~component, (_jsProps) => make([||]));
+let default = ReasonReact.wrapReasonForJs(~component, (jsProps) => make(~a=jsProps##a, ~b=jsProps##b, [||]));
+
+let getInitialProps = () => {
+  Js.Promise.make ((~resolve, ~reject as _) => {
+    resolve(.{"a": "test", "b": "b" });
+  })
+};
+
+let inject = [%bs.raw {| (cls, fn) => cls.getInitialProps = fn |}];
+inject(default, getInitialProps);

That said, I'm trying to explore some ways of doing this in a cleaner way as a wrapper on top of ReasonReact or additional next.js configuration which I'll share when I get there. In the meantime, the above is a similar trick to injecting to the imported component class in the JS file.

@tmepple
Copy link

tmepple commented Apr 30, 2018

I felt like there was a way to use the %bs.raw hack or something to get it done. It's pretty straightforward but if you find a cleaner way at some point that would be awesome. Thanks again for improving the with-reasonml example and getting back to me so fast.

lependu pushed a commit to lependu/next.js that referenced this pull request Jun 19, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Apr 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants