-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Add Reason asset type #342
Conversation
Nice! |
src/Parser.js
Outdated
@@ -12,6 +12,7 @@ class Parser { | |||
this.registerExtension('es6', './assets/JSAsset'); | |||
this.registerExtension('jsm', './assets/JSAsset'); | |||
this.registerExtension('mjs', './assets/JSAsset'); | |||
this.registerExtension('re', './assets/ReasonAsset'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about .ml
files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added for OCaml files too 😄
src/assets/ReasonAsset.js
Outdated
const fs = require('fs'); | ||
const JSAsset = require('./JSAsset'); | ||
const config = require('../utils/config'); | ||
const localRequire = require('../utils/localRequire'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appear to be unused:
const config = require('../utils/config');
const localRequire = require('../utils/localRequire');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, they were. Thanks!
src/assets/ReasonAsset.js
Outdated
// the Reason compilation process. | ||
await bsb.runBuild(); | ||
|
||
this.contents = await new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use ../utils/promisify
on fs.readFile
instead of new Promise
.
See parcel-plugin-bucklescript (npm) for an existing plugin. Similar implementation, but may provide a bit more on the config front. |
I just tried it out and I’m consistently getting the error:
And yes, I do have a file called hello.re:
Full output:
If you’re reproducing this, here’s the exact code I use (taken from here): hello.re
|
@brandon93s Thanks for the link! It looks like the implementation they used adapted from @davidnagli You'll need to run the following commands in your project to get set up with Reason:
and then copy this into {
"name": "reason-parcel-example",
"sources": [
"src"
],
"refmt": 3,
"package-specs": {
"module": "commonjs",
"in-source": true
},
"suffix": ".bs.js"
} |
Uhh, I don't think that last one should have failed. The HTML test failed for some reason? |
// This is a simplified use-case for Reason - it only loads the recommended | ||
// BuckleScript configuration to simplify the file processing. | ||
const outputFile = this.name.replace(/\.(re|ml)$/, '.bs.js'); | ||
const outputContent = await readFile(outputFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there no way to do this without writing to a temporary file first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, what do you mean by writing to a temp file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it seems like bsb-js
or whatever that uses is writing its output to a temp file, which we then need to read here. Is there a way to avoid that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. the compileFile
function as used by parcel-plugin-bucklescript seems to return the output directly to a variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compileFile basically does the same thing here — reading the result of the output. Currently BuckleScript has no way of compiling Reason like that without seriously compromised performance and maintainability.
@@ -14,6 +14,7 @@ | |||
"babylon": "^6.17.4", | |||
"babylon-walk": "^1.0.2", | |||
"browser-resolve": "^1.11.2", | |||
"bsb-js": "^1.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not take this on as a dependency - people can install it locally in their projects. can you move it to a dev dep for the tests and use localRequire
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bsb-js is a thin wrapper over finding and running a BuckleScript executable, kind of like the logic in the TS asset. I developed he package for use cases like this, adding Reason support easily to a new bundler.
This doesn’t actually depend on BuckleScript, so users would still have to install that themselves. Requiring users to install it themselves wouldn’t make that much sense because it’s a package meant to be used by tooling.
With this being the case, it is ok to add it as a dependency? If it helps at all, I’m the maintainer of that package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After #306 lands this won't be such a big deal since deps like this will be automatically installed by parcel as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even so, bsb-js
is still a pretty small dependency to add. It doesn't include the entire compiler toolchain and has no dependencies itself. The package should only add ~2.7k
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a slippery slope I think. We don't do it for other asset types, so I don't want to start now.
// Other Asset types use `localRequire` but the `bsb-js` package already | ||
// does that internally. This should also take care of error handling in | ||
// the Reason compilation process. | ||
if (process.env.NODE_ENV !== 'test') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not building in the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building during test would require adding BuckleScript as a dev dependency. BS is a big native binary that compiles itself on install, which takes about 7 minutes.
The wrapper around BuckleScript, bsb-js, is pretty well tested, so the tests for Reason only test the file-loading logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see, ok.
@@ -0,0 +1,11 @@ | |||
// Generated by BUCKLESCRIPT VERSION 1.9.2, PLEASE EDIT WITH CARE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't seem like this should be committed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hard coded the result of compiling the Reason file with BuckleScript so that we don’t have to run it when running the tests. BuckleScript is a binary that compiles itself during installation which could take a while. By hard coding the result we can test just the loading logic, because the logic to run BuckleScript is contained in another package, bsb-js.
@rrdelaney - That test experiences random failures; a fix for it is now in master. Sent with GitHawk |
@brandon93s Thanks 😄 I just pulled from upstream |
I moved bsb-js to a dev dependency and used localRequire. That should get it autoinstalled when used once #306 lands. |
* Add bsb-js dependency * Add ReasonAsset type * Actually add the Reason asset type * Add OCaml file type, add integration test, remote unused imports * use promisify for reading files * Fix integration tests
* Add bsb-js dependency * Add ReasonAsset type * Actually add the Reason asset type * Add OCaml file type, add integration test, remote unused imports * use promisify for reading files * Fix integration tests
This adds the Reason asset type to
.re
files.Right now this is pretty basic and only loads the recommended Reason configuration. I can update it to use other configurations if you want, but overall I think this is a good starting point.
The code for this is pretty simple -- a lot of the work was already done in the Reason commuity's wrapper for the build tool,
bsb-js
. That should handle requiring the build tool per-project and error handling already, so little logic other than "run the build" and "read the result" are needed.