-
Notifications
You must be signed in to change notification settings - Fork 58
BS in-source files supports #30
Comments
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. |
Sounds good. So how do you plan to proceed here? |
@rrdelaney I've been thinking about this and I was wondering if we actually need With the For example, I tried parceljs instead of // 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 I don't have the full picture with PS: this would also get rid of the |
Update: I can confirm that I can run a CRA project with ReasonML with only
"scripts": {
"start": "npm-run-all --parallel start:bsb start:app",
"start:app": "react-scripts start",
"start:bsb": "bsb -make-world -w",
} WDYT? |
So the plan is to leave There will be a time when |
Hmm ok so you still want to keep the Can we at least do following changes?
Also for me to understand. Can you list the actual differences between Thanks 🙏 |
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:
react-scripts
withreason-scripts
index.js
toindex.re
in-source
builds in conjunction with.bs.js
suffix (as recommended from the docs)One thing, being that there was a bug in
bs-loader
that would ignore thesuffix
. I've opened a PR for that rrdelaney/bs-loader#40However,
reason-scripts
still uses version1.8.0
ofbs-loader
and it should be upgraded to version2
. Not a huge deal, as this issue can be worked around using aresolutions
field withyarn
.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 fileHowever, 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:
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 😉
The text was updated successfully, but these errors were encountered: