Skip to content
This repository has been archived by the owner on Dec 3, 2018. It is now read-only.

BS in-source files supports #30

Closed
emmenko opened this issue Dec 25, 2017 · 6 comments
Closed

BS in-source files supports #30

emmenko opened this issue Dec 25, 2017 · 6 comments

Comments

@emmenko
Copy link

emmenko commented Dec 25, 2017

Hey guys,

I've started getting to know reason and reason-react in the last days, so I'm still totally new to the syntax/language and the complementary tooling such as bucklescript.

I do have a side project that I'm working on from time to time which is basically a simple CRA application. So I decided to start putting some reason flavour in it and as far as I could understand it it should be easy to migrate little pieces to reason incrementally.

Long story short:

  • I've replaced react-scripts with reason-scripts
  • I've migrated index.js to index.re
  • I've activated the new in-source builds in conjunction with .bs.js suffix (as recommended from the docs)
  • ...and I bumped into some small issues 🙃

One thing, being that there was a bug in bs-loader that would ignore the suffix. I've opened a PR for that rrdelaney/bs-loader#40
However, reason-scripts still uses version 1.8.0 of bs-loader and it should be upgraded to version 2. Not a huge deal, as this issue can be worked around using a resolutions field with yarn.
Still, I think reason-scripts should support this new options out of the box.

Here comes the additional missing feature: if I activate the in-source option, I'll end up having the compiled file .bs.js next to the source file

Foo.bs.js
Foo.re

However, the eslint rule used by webpack should exclude those auto generated files ending with .bs.js. Using a .eslintignore won't help because it's disabled on purpose on the rule.

A naive "fix" would be something like this:

- test: /\.(js|jsx|mjs)$/,
+ test: function testForJsFilesExcludingBsFiles(fileName) {
+   if (fileName.endsWith('.bs.js')) return false;
+   return /\.(js|jsx|mjs)$/.test(fileName);
+ },

So far those have been the "issues" that I have encountered while getting started with reason.

It would be nice to have them addressed, I'm not sure if there are other things that need to be improved though. But in general, having a good reason/bs support out of the box would be very nice.

Thanks a lot! 🙏

PS: I wanted to open an issue first to discuss those things, I'm also happy to provide a PR if you guys want to 😉

@rrdelaney
Copy link
Owner

Adding all that in a PR sounds good to me 😄 The reason I've been delaying adding bs-loader@2 is because I want to move away from it in the future with reason-scripts. bs-loader has some complexities that have been handled at the build-system level, like adding in-source builds. Ideally I'd like the have reason-scripts based off of that.

@emmenko
Copy link
Author

emmenko commented Dec 31, 2017

Sounds good. So how do you plan to proceed here?

@emmenko
Copy link
Author

emmenko commented Jan 4, 2018

@rrdelaney I've been thinking about this and I was wondering if we actually need reason-scripts at all. 🤔

With the in-source build feature, you get the compiled sources next to the original file. So in theory you can use whatever framework/bundler you want to start your app, you just need to import the *.bs.js files.
Obviously you would need to run bsb -make-world in watch mode to trigger the bundle rebuild after the *.bs.js file has been written.

For example, I tried parceljs instead of reason-scripts and the setup is very simple:

// package.json
"scripts": {
  "start": "npm-run-all --parallel start:bsb start:app",
  "start:app": "parcel -p 4000 public/index.html",
  "start:bsb": "bsb -make-world -w"
}

I think we can use react-scripts directly instead of maintaining a "copy" of it. Of course you need to setup a couple more things (e.g. bucklescript stuff) but if it's good documented there shouldn't be any problem with it.

I don't have the full picture with reason-scripts, so I might miss some crucial point, but what do you think?

PS: this would also get rid of the bs-loader package like you wanted.

@emmenko
Copy link
Author

emmenko commented Jan 8, 2018

Update: I can confirm that I can run a CRA project with ReasonML with only react-scripts.

  • create a index.js file that imports the index.bs.js file generated by your "normal" entry point index.re
  • run bsb in watch mode as a parallel process
"scripts": {
  "start": "npm-run-all --parallel start:bsb start:app",
  "start:app": "react-scripts start",
  "start:bsb": "bsb -make-world -w",
}

WDYT?

@rrdelaney
Copy link
Owner

So the plan is to leave reason-scripts as a stable and fast way to get started with ReasonReact. I understand there are other ways to get started and use RR, but people from the React community are well versed with CRA. I'd rather use the infrastructure of CRA and maintain a version of react-scripts than create a custom tool to set up webpack and everything with a complex config.

There will be a time when reason-scripts switches to in-source in the future, but there is no immediately compelling reason.

@emmenko
Copy link
Author

emmenko commented Jan 9, 2018

Hmm ok so you still want to keep the bs-loader in?

Can we at least do following changes?

Also for me to understand. Can you list the actual differences between reason-scripts and react-scripts?

Thanks 🙏

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

No branches or pull requests

2 participants