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

Read bsconfig instead of webpack options #27

Merged
merged 6 commits into from
Sep 18, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
const { readBsConfigSync } = require('read-bsconfig')
const path = require('path')
const { readFile, readFileSync } = require('fs')
const { execFile, execFileSync } = require('child_process')
Expand Down Expand Up @@ -89,11 +90,29 @@ const getCompiledFileSync = (moduleDir, path) => {
return transformSrc(moduleDir, res.toString())
}

const getBsConfigModuleOptions = buildDir => {
Copy link
Owner

Choose a reason for hiding this comment

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

Can this be async instead of blocking? The loader is run for everyfile and this would be a lot of overhead

const bsconfig = readBsConfigSync(buildDir)
Copy link
Owner

Choose a reason for hiding this comment

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

It might be a good idea to cache the result of this function. It's not expected to change for the process of the build, and this is run once per file for every build

Copy link
Owner

Choose a reason for hiding this comment

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

read-bsconfig will cache a bsconfig for us now so it doesn't read more than once

if (!bsconfig) {
throw new Error(`bsconfig not found in ${buildDir}`)
}

if (!bsconfig['package-specs'] || !bsconfig['package-specs'].length) {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think package-specs needs to be defined, doesn't it default to using CommonJS?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh that's right. If it's not defined i'll default to 'js' like we do in the options part 👍

throw new Error('package-specs not defined in bsconfig')
}

const moduleSpec = bsconfig['package-specs'][0]
const module = typeof moduleSpec === 'string' ? moduleSpec : moduleSpec.module
const inSource =
typeof moduleSpec === 'string' ? false : moduleSpec['in-source']
return { module, inSource }
}

module.exports = function loader() {
const options = getOptions(this) || {}
const buildDir = options.cwd || CWD
const moduleDir = options.module || 'js'
const inSourceBuild = options.inSource || false
const bsconfig = getBsConfigModuleOptions(buildDir)
const moduleDir = bsconfig.module || 'js'
Copy link
Owner

Choose a reason for hiding this comment

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

So this isn't a breaking change, can we add the options passed in from the Webpack config as the first choice? Something like:

const moduleDir = options.module || bsconfig.module || 'js'
const inSourceBuild = options.inSource || bsconfig.inSource || false

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well technically it's not breaking since the options would just be ignored and if your options and bsconfig don't match bs-loader won't work today right?

But I agree having the options there explicitly first is a safer change 👍 I'll add that back

Copy link
Owner

Choose a reason for hiding this comment

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

Is it a breaking change to make peoples' breaking builds work 🤔

But yea, I think it's a safer option too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One thing about that though is that then the defaults from reason-scripts will stay in place so in-source builds for that wouldn't work. Unless a new version of reason-scripts with this change would just remove the webpack options as well 👍

Copy link
Owner

Choose a reason for hiding this comment

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

Yea, I think we should push that up on the reason-scripts side

const inSourceBuild = bsconfig.inSource || false

this.addContextDependency(this.context)
const callback = this.async()
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@
},
"homepage": "https://github.com/rrdelaney/bs-loader#readme",
"dependencies": {
"loader-utils": "^1.1.0"
"loader-utils": "^1.1.0",
"read-bsconfig": "^1.0.0"
},
"devDependencies": {
"jest": "^21.0.1",
Expand Down
1 change: 0 additions & 1 deletion test/simple/webpack.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ const baseConfig = {
use: {
loader,
options: {
module: 'es6',
cwd: __dirname
}
}
Expand Down