-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add Reason asset type #342
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
Changes from all commits
7cb5164
e864f7c
565b89a
56cd8ef
66e24fa
3fcd205
51d65ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
const bsb = require('bsb-js'); | ||
const fs = require('fs'); | ||
const JSAsset = require('./JSAsset'); | ||
const promisify = require('../utils/promisify'); | ||
const readFile = promisify(fs.readFile); | ||
|
||
class ReasonAsset extends JSAsset { | ||
async parse(code) { | ||
// This runs BuckleScript - the Reason to JS compiler. | ||
// 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. i see, ok. |
||
await bsb.runBuild(); | ||
} | ||
|
||
// 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I mean, it seems like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. e.g. the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
this.contents = outputContent.toString(); | ||
|
||
// After loading the compiled JS source, use the normal JS behavior. | ||
return await super.parse(this.contents); | ||
} | ||
} | ||
|
||
module.exports = ReasonAsset; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
var local = require('./local.re'); | ||
|
||
module.exports = function () { | ||
return local.a + local.b; | ||
}; |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
let a = 1; | ||
|
||
let b = 2; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
const assert = require('assert'); | ||
const fs = require('fs'); | ||
const {bundle, run, assertBundleTree} = require('./utils'); | ||
|
||
describe('reason', function() { | ||
it('should produce a bundle', async function() { | ||
let b = await bundle(__dirname + '/integration/reason/index.js'); | ||
|
||
assert.equal(b.assets.size, 2); | ||
assert.equal(b.childBundles.size, 0); | ||
|
||
let output = run(b); | ||
assert.equal(typeof output, 'function'); | ||
assert.equal(output(), 3); | ||
}); | ||
}); |
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.7kThere 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.