Read bsconfig instead of webpack options #27
Conversation
index.js
Outdated
| @@ -89,11 +90,29 @@ const getCompiledFileSync = (moduleDir, path) => { | |||
| return transformSrc(moduleDir, res.toString()) | |||
| } | |||
|
|
|||
| const getBsConfigModuleOptions = buildDir => { | |||
There was a problem hiding this comment.
Can this be async instead of blocking? The loader is run for everyfile and this would be a lot of overhead
index.js
Outdated
| @@ -89,11 +90,29 @@ const getCompiledFileSync = (moduleDir, path) => { | |||
| return transformSrc(moduleDir, res.toString()) | |||
| } | |||
|
|
|||
| const getBsConfigModuleOptions = buildDir => { | |||
| const bsconfig = readBsConfigSync(buildDir) | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
read-bsconfig will cache a bsconfig for us now so it doesn't read more than once
index.js
Outdated
| throw new Error(`bsconfig not found in ${buildDir}`) | ||
| } | ||
|
|
||
| if (!bsconfig['package-specs'] || !bsconfig['package-specs'].length) { |
There was a problem hiding this comment.
I don't think package-specs needs to be defined, doesn't it default to using CommonJS?
There was a problem hiding this comment.
Oh that's right. If it's not defined i'll default to 'js' like we do in the options part 👍
index.js
Outdated
| const moduleDir = options.module || 'js' | ||
| const inSourceBuild = options.inSource || false | ||
| const bsconfig = getBsConfigModuleOptions(buildDir) | ||
| const moduleDir = bsconfig.module || 'js' |
There was a problem hiding this comment.
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 || falseThere was a problem hiding this comment.
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
There was a problem hiding this comment.
Is it a breaking change to make peoples' breaking builds work 🤔
But yea, I think it's a safer option too
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
Yea, I think we should push that up on the reason-scripts side
|
|
This changes the loader to read the bsconfig instead of having separate webpack options which need to stay in sync with bsconfig.json.
I developed this with this version of read-bsconfig so when that PR has been merged, I can bump the version of the dependency here and it should be a non breaking change then.