make the .bootstraprc location customizable

[pherris]

I needed the option to run multiple variations of the .bootstraprc file against the same project. This fork lets developers pass in the location of their .bootstraprc file and cleans up some of the inter-dependencies between bootstrap.loader.js and bootstrap.config.js around creation of the config object.

Also should address #75.

---

This change is

[pherris]

Forgot to mention in the description that this PR also allows use of npm link for development. Documentation was updated to reflect this change.

[justin808]

@pherris This looks really good. I'd like to see this applied to the some examples with the docs referring to that. I'll use that to manually test. Of course, bonus points if you could lead the way and get us going with some automated tests.

CC: @alexfedoseev

[pherris]

got the basic test framework added, but needs some real tests... Copied directory structure from the react_on_rails repo.

[pherris]

@justin808 I need to make a call on whether or not to start using my fork instead of this repo - do you think merging and releasing this PR is in the near future? Is there anything else you see needing to be completed?

[yuki-takei]

+1 looking forward to merging

[shusugmt]

+1

[outdooricon]

@justin808 love the project and the work you guys are doing! I am wondering what's the hold up on this pr? I'd like to see this come in and it looks like all of the conditions for merge were met... Is there something else that needs to be done, or can we get this approved? Thanks!

[justin808]

@pherris This looks really good. I'd like to see this applied to the some examples with the docs referring to that. I'll use that to manually test. Of course, bonus points if you could lead the way and get us going with some automated tests.

That's what I need to merge this. We just need a sample in this repo that I can pull and verify the change.

Also, if others besides the author have verified the change, that would be great.

@alexfedoseev and I are slammed working on our new product.

CC: @outdooricon, @s2ugimot, @yuki-takei, @pherris

[justin808]

@pherris @yuki-takei @s2ugimot @outdooricon I'd like to get this one merged. Please see my prior comment.

[pherris]

Sorry, I was slammed at work last week. I'll get the examples updated early
next week - thx!

On Saturday, June 25, 2016, Justin Gordon [email protected] wrote:

@pherris https://github.com/pherris @yuki-takei
https://github.com/yuki-takei @s2ugimot https://github.com/s2ugimot
@outdooricon https://github.com/outdooricon I'd like to get this one
merged. Please see my prior comment.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#90 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AB_O9uqWtIXK2o_Hiz3ZdDpoRcsXiHGIks5qPbiSgaJpZM4Il98I
.

[lo1tuma]

That feature would be extremely helpful for me 👍.
Is there anything I could help with to get this feature released?

[justin808]

@lo1tuma Please see the conversation above. I just need a PR that has this.

[pherris]

I updated the basic example but was not able to fully test it due to the interdependencies between the basic example which depends on bootstrap-loader and had to use my local, npm linkd bootstrap-loader. I believe this example will work it does not have to use the npm linkd version of bootstrap loader but I'm not 100%.

The symptom I saw was having to npm link all of the loaders from the bootstrap-loader/examples/basic/node_modules/ to bootstrap-loader/node_modules - once I had them all linked, I ran into the Error: "extract-text-webpack-plugin" loader is used without the corresponding plugin error which seems to be due to how node is handling symlinked modules. Once I ran into this I thought it'd be easier to fall forward with the PR and (if needed) fix the example after the library is released so the example code can be worked in isolation from the changes to the library itself.

The basic test code was added some time ago, so @justin808 I believe this was the only outstanding item.

[justin808]

@pherris FYI -- it's pretty common to have issues with npm link. I'll try this out.

[justin808]

I have a few questions. I'm going to ask @alexfedoseev to review and mark the files reviewed once he's OK with the changes.

---

Review status: 0 of 11 files reviewed at latest revision, 6 unresolved discussions.

---

CONTRIBUTING.md, line 20 [r4] (raw file):

It will run linters, clear directory with previous build, create new build and run watchers to re-build on every change. We can use the npm link feature in our development process if we reference full paths to our loader in webpack's config: bootstrap-loader/lib/bootstrap.loader?extractStyles&configFilePath=${__dirname}/.bootstraprc!bootstrap-loader/no-op.js. In order for this library to find the expected bootstrap version, you must also npm link the expected bootstrap and extract-text-webpack-plugin versions from your project's node_modules directory to your clone of this library.

@pherris Given your last comment on this issue, should we change this part of the README.md?

---

examples/basic/.bootstraprc, line 1 [r4] (raw file):

---

Why was this file deleted?

---

examples/basic/.gitignore, line 220 [r4] (raw file):

*.lnk

.bootstraprc

Why are you adding this to .gitignore?

---

examples/basic/package.json, line 12 [r4] (raw file):

    "clean": "rm -rf public/",
    "build": "npm run clean && webpack --config webpack.prod.config.js",
    "bs3": "nodemon --watch app/markup --ext html server.dev.js --BOOTSTRAPRC_LOCATION=.bootstraprc-3-example",

Why are we using all caps for the option?

Why not: --boostraprc-location

Note, man tar shows options using a dash.

---

examples/basic/webpack.dev.config.js, line 7 [r4] (raw file):

const path = require('path');
const autoprefixer = require('autoprefixer');
const BOOTSTRAPRC_LOCATION = process.argv.find(val => val.includes('--BOOTSTRAPRC_LOCATION')).split('=')[1];

Do we need to have this in all caps?

const boostraprcLocation = process.argv.find(val => val.includes('--bootstraprc-location')).split('=')[1];

---

examples/basic/webpack.dev.config.js, line 24 [r4] (raw file):

      'bootstrap-loader/lib/bootstrap.loader?',
      `extractStyles&configFilePath=${__dirname}/${BOOTSTRAPRC_LOCATION}`,
      '!bootstrap-loader/no-op.js'

Why the other changes here?

---

Comments from Reviewable

[justin808]

I tried the following and got errors. I did be sure to run npm i before running this.

➜  ~/shakacode/bootstrap-loader/examples/basic (pherris-master) npm run bs3:dev                                                                                         [22:01:27]

> [email protected] bs3:dev /Users/justin/shakacode/bootstrap-loader/examples/basic
> npm run bs3


> [email protected] bs3 /Users/justin/shakacode/bootstrap-loader/examples/basic
> nodemon --watch app/markup --ext html server.dev.js --BOOTSTRAPRC_LOCATION=.bootstraprc-3-example

[nodemon] 1.9.1
[nodemon] to restart at any time, enter `rs`
[nodemon] watching: /Users/justin/shakacode/bootstrap-loader/examples/basic/app/markup/**/*
[nodemon] starting `node server.dev.js --BOOTSTRAPRC_LOCATION=.bootstraprc-3-example`
=> 🔥  Webpack dev server is running on port 4000
webpack built b2c8ea8dd96a032b9da2 in 2183ms
Time: 2183ms
                                 Asset     Size  Chunks       Chunk Names
  32400f4e08932a94d8bfd2422702c446.eot  70.8 kB
db812d8a70a4e88e888744c1c9a27e89.woff2  66.6 kB
 a35720c2fed2c7f043bc7e4ffb45e073.woff  83.6 kB
  a3de2170e4e9df77161ea5d3f31b2668.ttf   142 kB
  f775f9cca88e21d45bebe185b27c0e5b.svg   366 kB
                                app.js  1.26 MB       0       main

ERROR in ./~/extract-text-webpack-plugin/loader.js?{"omit":1,"extract":true,"remove":true}!./~/style-loader!./~/css-loader!./~/resolve-url-loader!./~/sass-loader?sourceMap!./~/bootstrap-loader/lib/bootstrap.styles.loader.js!./~/bootstrap-loader/no-op.js
Module build failed: Error: "extract-text-webpack-plugin" loader is used without the corresponding plugin, refer to https://github.com/webpack/extract-text-webpack-plugin for the usage example
    at Object.module.exports.pitch (/Users/justin/shakacode/bootstrap-loader/examples/basic/node_modules/extract-text-webpack-plugin/loader.js:21:9)
 @ ./~/bootstrap-loader/lib/bootstrap.loader.js?extractStyles&configFilePath=/Users/justin/shakacode/bootstrap-loader/examples/basic/.bootstraprc-3-example!./~/bootstrap-loader/no-op.js 1:0-245
webpack: bundle is now VALID.

---

Review status: 0 of 11 files reviewed at latest revision, 6 unresolved discussions.

---

Comments from Reviewable

[justin808]

@pherris I'm guessing that we should be testing this by installing locally. I used to just release to npm to do some testing, but it's really not necessary.

---

Review status: 0 of 11 files reviewed at latest revision, 7 unresolved discussions.

---

a discussion (no related file):
@outdooricon, @s2ugimot, @yuki-takei If any of you can try this out and comment, and put comments in the review. that would be great.

---

Comments from Reviewable

[pherris]

sheesh - you guys aren't using this plugin in this example. I'll need to look into when/why that made it into the code - is it being used in the downstream webpack_react_on_rails code maybe? In the mean time I'll update this pr.

---

Review status: 0 of 11 files reviewed at latest revision, 8 unresolved discussions.

---

CONTRIBUTING.md, line 20 [r4] (raw file):

Previously, justin808 (Justin Gordon) wrote…

@pherris Given your last comment on this issue, should we change this part of the README.md?

Well, it seems not all implementations require the `extract-text-webpack-plugin` - I'll need to further grock where this plugin is applicable and potentially update this.

---

examples/basic/.bootstraprc, line 1 [r4] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Why was this file deleted?

I refactored the two methods in this basic example to use their respective .bootstraprc files instead of removing this file and copying a different .bootstraprc into it's place. I also think the example is more clear without it since you are not really using the checked in version of this file in the example.

Since this PR enables us to use custom .bootstraprc files, we can use the .bootstraprc-3-example and .bootstraprc-4-example files directly.

---

examples/basic/.gitignore, line 220 [r4] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Why are you adding this to .gitignore?

To make it totally clear that we are not using this default file - the example never really was using the checked-in version of this file as far as I can tell. Each variation was copied over this file at runtime.

---

examples/basic/package.json, line 12 [r4] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Why are we using all caps for the option?

Why not: --boostraprc-location

Note, man tar shows options using a dash.

This can be moved to lower case - I was playing with an environment variable at first and I never changed to lower case when I decided to pass to the process.

---

examples/basic/webpack.dev.config.js, line 7 [r4] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Do we need to have this in all caps?

const boostraprcLocation = process.argv.find(val => val.includes('--bootstraprc-location')).split('=')[1];

we can move to lower case/camelCase - I will make this update.

---

examples/basic/webpack.dev.config.js, line 24 [r4] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Why the other changes here?

to pass the `configFilePath` into the loader - the extractStyles option should no longer be required - looking into extractStyles further...

---

Comments from Reviewable

[pherris]

To test locally I ran npm install in the root directory. Next, I cd examples/basic/node_modules/bootstrap-loader and rm -fr lib followed by cp -r ../../../../lib . to get the currently checked out & built version of bootstrap-loader into the example. Finally, npm run bs4:dev from examples/basic and open http://localhost:4000 FTW!

[pherris]

the comments about extract-text-webpack-plugin are still valid if you are using bootstrap.loader?extractStyles& - updated CONTRIBUTING readme slightly.

[justin808]

@pherris

1. We still need an example that shows the "default" behavior. The idea is that since we don't have really good automated tests, we should be able run just a few simple steps and ensure that the project is working correctly. We actually need to document that.
2. Regarding the extract styles plugin, it's super relevant for production mode. In production, we don't do hot reloading and we often want the styles in a separate CSS file.

[pherris]

@justin808 I think the PR as it stands is working well with extractStyles and the comments are correct - do you see any issues on that front?

I will push another commit with a separate example using the customized location (mostly the same as the basic example).

[justin808]

Review status: 0 of 27 files reviewed at latest revision, 14 unresolved discussions.

---

a discussion (no related file):

Previously, justin808 (Justin Gordon) wrote…

@outdooricon, @s2ugimot, @yuki-takei If any of you can try this out and comment, and put comments in the review. that would be great.

I've got a few more comments. @alexfedoseev will need to review this as he rewrote my original version. Please be patient as @alexfedoseev is on a critical assignment right now.

@pherris Please see if you can apply my suggested example changes:

1. Don't duplicate the test directory, instead using different scripts to start.
2. Have the production examples use extract styles.

@pherris if there's any npm modules you can also update, such as at the top level or in the examples, that would be greatly appreciated by @alexfedoseev and me.

To all, @alexfedoseev and I are slammed on our other projects, so we appreciate your patience.

---

CONTRIBUTING.md, line 20 [r4] (raw file):

Previously, pherris (Andrew Ralston) wrote…

Well, it seems not all implementations require the extract-text-webpack-plugin - I'll need to further grock where this plugin is applicable and potentially update this.

Hi @pherris The steps here are not clear.

I'd like to put the instructions for using npm link under a separate header, and then I'll try them out. Additionally, I don't understand the requirement to use npm link to bootstrap and the extract-text-plugin. I'm guessing that is due to the way that this loader finds the bootstrap source to use.

Nevertheless, let's put this in a numbered list of steps.

---

package.json, line 62 [r7] (raw file):

    "eslint-config-airbnb": "^2.0.0",
    "tap-spec": "^4.1.1",
    "tape": "^4.5.1"

I'd be thrilled if you'd bring all the dependencies to current, and also swap out the eslint-config-airbnb with the https://www.npmjs.com/package/eslint-config-shakacode.

---

README.md, line 64 [r7] (raw file):

Config is optional. If used, by default it should be placed in your project's root dir with name `.bootstraprc`. You can write it in `YAML` or `JSON` formats. Take a look at the default config files for [Bootstrap 3](.bootstraprc-3-default) and [Bootstrap 4](.bootstraprc-4-default). Note, we recommend using a configuration or else you might pick up unwanted upgrades, such as when we make Bootstrap 4 the default. Config options don't fall back on the defaults once a config file is present. Be sure not to delete config options. To start with a custom config, copy over a default config file as a starting point.

If the default location doesn't work for you (e.g. you want to create multiple bootstrap configs for branding or you symlink your npm_modules directory), you may pass the absolute path of the `.bootstraprc` file to the loader in your webpack config, e.g. `bootstrap-loader/lib/bootstrap.loader?extractStyles&configFilePath=${__dirname}/.bootstraprc!bootstrap-loader/no-op.js`.

We might want to clarify that ${__dirname}/.bootstraprc!bootstrap-loader/no-op.js is an absolute path and how __dirname is resolved.

Also, why do we need to put in the extractStyles here? Normally, we specify this in the config file

---

README.md, line 111 [r7] (raw file):

* Basic usage: [examples/basic](examples/basic)
* With CSS Modules: [examples/css-modules](examples/css-modules) (This example shows off hot reloading with Babel 6 as well!)
* With custom .bootstraprc location/name: [examples/custom-bootstraprc-location](examples/custom-bootstraprc-location)

Could we have different scripts in package.json rather than duplicating the directory structure of the examples/basic and examples/custom-bootstrap-location? Seems like a lot of maintenance work to have duplicated files.

Another way around the duplicated files is to use a relative sym link added to the git project. "relative" is key.

---

examples/basic/.bootstraprc, line 6 [r7] (raw file):

# Major version of Bootstrap: 3 or 4
bootstrapVersion: 4

I'm not sure if we should change the default to 4 given that bootstrap 4 is still not yet released.

I do like the idea of having the script copy this file from the source and having this exact path in this directory be git ignored. In that case, this file is just deleted.

@alexfedoseev, thoughts?

---

examples/basic/.bootstraprc, line 28 [r7] (raw file):

#     extractStyles: false
#   production:
#     extractStyles: true

Possibly we should pass this in the npm script for production, as this is more realistic.

---

examples/custom-bootstraprc-location/.bootstraprc-3-example, line 1 [r7] (raw file):

---

I really don't like the duplication of all these files.

See my previous comment about having different scripts.

---

src/bootstrap.config.js, line 15 [r7] (raw file):

const SUPPORTED_VERSIONS = [3, 4];
const CONFIG_FILE = '.bootstraprc';
const defaultUserConfigPath = `../../../${CONFIG_FILE}`;

Why did this change?

This will be a partial path.

Before we had a full (absolute?) path.

I see, this is used below as a parameter, and resul

---

src/bootstrap.config.js, line 68 [r7] (raw file):

  const configFile = path.resolve(__dirname, configFilePath);

  if (userConfigFileExists(configFile)) {

What is the else condition? the defaults of no file?

---

src/bootstrap.config.js, line 101 [r7] (raw file):

  return {
    bootstrapVersion: parseInt(rawConfig.bootstrapVersion, 10),
    loglevel: rawConfig.loglevel,

was this a prior bug, @alexfedoseev?

---

examples/basic/.gitignore, line 220 [r4] (raw file):

Previously, pherris (Andrew Ralston) wrote…

To make it totally clear that we are not using this default file - the example never really was using the checked-in version of this file as far as I can tell. Each variation was copied over this file at runtime.

I think this is a good idea, but, the run script copies this over, and we need to mention it here.

---

examples/basic/webpack.dev.config.js, line 24 [r4] (raw file):

Previously, pherris (Andrew Ralston) wrote…

to pass the configFilePath into the loader - the extractStyles option should no longer be required - looking into extractStyles further...

We'll need to check with @alexfedoseev

---

Comments from Reviewable

[justin808]

@pherris Let me know if you want to screen share and discuss any of the comments. I'm eager to get this one wrapped up (and not distract @alexfedoseev too much).

[justin808]

@pherris Any update on this one?

[alex35mil]

Sorry, guys, I need to close few more cards before I'll get to this PR. Hope to review it in the next couple of days.

[pherris]

Review status: 0 of 27 files reviewed at latest revision, 14 unresolved discussions.

---

a discussion (no related file):

Previously, justin808 (Justin Gordon) wrote…

I've got a few more comments. @alexfedoseev will need to review this as he rewrote my original version. Please be patient as @alexfedoseev is on a critical assignment right now.

@pherris Please see if you can apply my suggested example changes:

1. Don't duplicate the test directory, instead using different scripts to start.
2. Have the production examples use extract styles.

@pherris if there's any npm modules you can also update, such as at the top level or in the examples, that would be greatly appreciated by @alexfedoseev and me.

To all, @alexfedoseev and I are slammed on our other projects, so we appreciate your patience.

@justin808 - I'm only struggling with the feedback on the examples (how to share the code between the examples while not substantially modifying the other examples to make it work) - it would be good to discuss if you have time. I'm on your slack channel if you want to message me there to set something up. I have Jury duty this week so I'm not 100% sure when I'll be online but will try to check in late my time (Central) since you are behind my time by several hours.

---

CONTRIBUTING.md, line 20 [r4] (raw file):

structions for using npm link under a separate header, and then I'll try them out. Additionally, I don't understand the requirement to use npm link to bootstrap and the extract-text-plugin. I'm guessing that is due to the way that this loader finds t

Updating like this implies that you would do development without using npm link - but why would anyone use another way? (I am pushing a change however) The npm link of bootstrap is due to how bootstrap files are located: https://github.com/pherris/bootstrap-loader/blob/master/src/bootstrap.loader.js#L82

---

package.json, line 62 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

I'd be thrilled if you'd bring all the dependencies to current, and also swap out the eslint-config-airbnb with the https://www.npmjs.com/package/eslint-config-shakacode.

sure...

---

README.md, line 64 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

We might want to clarify that ${__dirname}/.bootstraprc!bootstrap-loader/no-op.js is an absolute path and how __dirname is resolved.

Also, why do we need to put in the extractStyles here? Normally, we specify this in the config file

It does mention that `.bootstraprc` is absolute - will make the clarification on where `${__dirname}` comes from. For `extractStyles` this method replaces https://github.com/shakacode/bootstrap-loader/blob/2fadab2ddae2e33abee648a598bcc4c92e91f9e3/extractStyles.js#L1 which is called from the prod version of the webpack file here: https://github.com/shakacode/bootstrap-loader/blob/2fadab2ddae2e33abee648a598bcc4c92e91f9e3/examples/basic/webpack.prod.config.js#L15 - I'm open to clarifying this... can you suggest something?

---

examples/basic/.bootstraprc, line 6 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

I'm not sure if we should change the default to 4 given that bootstrap 4 is still not yet released.

I do like the idea of having the script copy this file from the source and having this exact path in this directory be git ignored. In that case, this file is just deleted.

@alexfedoseev, thoughts?

That would clean this up - definitely. I don't see any reason not to (and had included that change when the two examples were together) - I'll do that.

---

src/bootstrap.config.js, line 15 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Why did this change?

This will be a partial path.

Before we had a full (absolute?) path.

I see, this is used below as a parameter, and resul

just in case they don't supply the location it'll default to this.

---

src/bootstrap.config.js, line 68 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

What is the else condition? the defaults of no file?

since I `return` in the `if` the else is just the next `return` statement

---

Comments from Reviewable

[justin808]

Review status: 0 of 29 files reviewed at latest revision, 12 unresolved discussions.

---

a discussion (no related file):

Previously, pherris (Andrew Ralston) wrote…

@justin808 - I'm only struggling with the feedback on the examples (how to share the code between the examples while not substantially modifying the other examples to make it work) - it would be good to discuss if you have time. I'm on your slack channel if you want to message me there to set something up. I have Jury duty this week so I'm not 100% sure when I'll be online but will try to check in late my time (Central) since you are behind my time by several hours.

Sure. We'll chat on Slack. Basically, we need to have on set of npm scripts that set an ENV var or program param to specify the position in the webpack file, or else specify nothing and be sure that we copied the file to the default place.

Let's get this done before @alexfedoseev gets involved.

---

CONTRIBUTING.md, line 20 [r4] (raw file):

Previously, pherris (Andrew Ralston) wrote…

structions for using npm link under a separate header, and then I'll try them out. Additionally, I don't understand the requirement to use npm link to bootstrap and the extract-text-plugin. I'm guessing that is due to the way that this loader finds t

Updating like this implies that you would do development without using npm link - but why would anyone use another way? (I am pushing a change however) The npm link of bootstrap is due to how bootstrap files are located: https://github.com/pherris/bootstrap-loader/blob/master/src/bootstrap.loader.js#L82

I prefer npm link, but we had issues before. Maybe they are solved with node v6.

---

README.md, line 64 [r7] (raw file):

Previously, pherris (Andrew Ralston) wrote…

It does mention that .bootstraprc is absolute - will make the clarification on where ${__dirname} comes from. For extractStyles this method replaces

bootstrap-loader/extractStyles.js

Line 1 in 2fadab2

require('./lib/bootstrap.loader?extractStyles!./no-op.js');

which is called from the prod version of the webpack file here:

bootstrap-loader/examples/basic/webpack.prod.config.js

Line 15 in 2fadab2

'bootstrap-loader/extractStyles',

- I'm open to clarifying this... can you suggest something?

I think code below is OK, however, put **absolute path** in bold!

---

README.md, line 111 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Could we have different scripts in package.json rather than duplicating the directory structure of the examples/basic and examples/custom-bootstrap-location? Seems like a lot of maintenance work to have duplicated files.

Another way around the duplicated files is to use a relative sym link added to the git project. "relative" is key.

@pherris please mark discussion as resolved if you did this. Also, I thought that we're going to remove the duplication.

---

examples/basic/.gitignore, line 221 [r8] (raw file):

# the npm run ... commands can create this file but we don't want it checked in
.bootstraprc

OK -- consider making this at top level. I believe

/.bootstraprc would do it.

---

examples/basic/package.json, line 14 [r8] (raw file):

    "build": "npm run clean && webpack --config webpack.prod.config.js",
    "prod": "npm run build && nodemon --watch app/markup --ext html server.prod.js",
    "bs3": "rm -f .bootstraprc && cp .bootstraprc-3-example .bootstraprc",

cp -f and you would not need the rm -f, right?

---

examples/basic/package.json, line 19 [r8] (raw file):

    "bs4:dev": "npm run bs4 && npm start",
    "bs3:prod": "npm run bs3 && npm run prod",
    "bs4:prod": "npm run bs4 && npm run prod"

We should have options here for specifying the path.

---

examples/custom-bootstraprc-location/.bootstraprc-3-example, line 1 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

I really don't like the duplication of all these files.

See my previous comment about having different scripts.

@pherris What's the plan for this?

---

examples/basic/webpack.dev.config.js, line 24 [r4] (raw file):

Previously, justin808 (Justin Gordon) wrote…

We'll need to check with @alexfedoseev

@alexfedoseev ?

---

Comments from Reviewable

[justin808]

Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

examples/custom-bootstraprc-location/webpack.prod.config.js, line 15 [r8] (raw file):

The same way as it's done in development config?
Not sure what exactly you mean, @alexfedoseev

---

Comments from Reviewable

[pherris]

I'm getting a little PR-review fatigue & had some other work to catch up on. Will review again now.

---

Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

CONTRIBUTING.md, line 20 [r4] (raw file):

Previously, justin808 (Justin Gordon) wrote…

I prefer npm link, but we had issues before. Maybe they are solved with node v6.

what can I say - it worked perfectly for me...

---

examples/basic/.gitignore, line 220 [r4] (raw file):

Previously, justin808 (Justin Gordon) wrote…

I think this is a good idea, but, the run script copies this over, and we need to mention it here.

Done.

---

examples/basic/.gitignore, line 221 [r8] (raw file):

Previously, justin808 (Justin Gordon) wrote…

OK -- consider making this at top level. I believe

/.bootstraprc would do it.

Done.

---

examples/basic/package.json, line 14 [r8] (raw file):

Previously, justin808 (Justin Gordon) wrote…

cp -f and you would not need the rm -f, right?

Done.

---

examples/basic/webpack.dev.config.js, line 24 [r4] (raw file):

Previously, alexfedoseev (Alex Fedoseev) wrote…

This file now is identical to the one on master. There were a changes during PR, but they are reverted now.

There were some comments about including the configurable location with the basic example being too convoluted so the customizable location example was moved to a different directory and this file was reverted to it's original state.

---

Comments from Reviewable

[pherris]

Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

README.md, line 111 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

@pherris please mark discussion as resolved if you did this. Also, I thought that we're going to remove the duplication.

Still needs to be done...

---

src/bootstrap.config.js, line 101 [r7] (raw file):

Previously, alexfedoseev (Alex Fedoseev) wrote…

Previously it was defined above: http://share.minima.pro/gigX

Done.

---

Comments from Reviewable

[pherris]

Review status: 7 of 12 files reviewed at latest revision, 14 unresolved discussions.

---

a discussion (no related file):

Previously, justin808 (Justin Gordon) wrote…

We still need to remove the duplication in the examples, and a few comments need to be addressed by @pherris.

Otherwise, looking good!

I took another swing at tightening up the examples - I'm back to using the basic examples but with new npm run entry points. Let me know what you think... Maybe in future PRs the scope can be smaller so the review process isn't so painful :(

---

examples/basic/package.json, line 19 [r8] (raw file):

Previously, justin808 (Justin Gordon) wrote…

@alexfedoseev @pherris to avoid the duplication of the examples. Just one example dir that covers all cases. So additional NPM scripts here with different parameters.

Done.

---

examples/custom-bootstraprc-location/.bootstraprc-3-example, line 1 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

@pherris How's this task going?

this directory was removed and examples integrated with the basic example

---

examples/custom-bootstraprc-location/webpack.prod.config.js, line 15 [r8] (raw file):

Previously, justin808 (Justin Gordon) wrote…

The same way as it's done in development config?
Not sure what exactly you mean, @alexfedoseev

The dev config accepts the arguments because the server.dev.js calls the webpack process but the server.prod.js relies on the files having already been built via `npm run build` so the arguments have to be passed differently. The basic example has this built in now if you want to check it out.

---

Comments from Reviewable

[pherris]

Review status: 7 of 12 files reviewed at latest revision, 14 unresolved discussions.

---

examples/basic/.bootstraprc, line 28 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Possibly we should pass this in the npm script for production, as this is more realistic.

please check the latest on the PR - you will see this in the `npm run build` task (when called from `bs3/4:customlocation:prod`)

---

examples/custom-bootstraprc-location/webpack.prod.config.js, line 15 [r8] (raw file):

Previously, pherris (Andrew Ralston) wrote…

The dev config accepts the arguments because the server.dev.js calls the webpack process but the server.prod.js relies on the files having already been built via npm run build so the arguments have to be passed differently. The basic example has this built in now if you want to check it out.

clarification here, `npm run build` is called from `bs3/4:customlocation:prod` which is where the argument is passed in via the `-- --bootstraprc-location=.bootstraprc-3-example` npm convention.

---

Comments from Reviewable

[justin808]

Reviewed 2 of 4 files at r1, 1 of 3 files at r2, 1 of 1 files at r3, 3 of 6 files at r8, 7 of 24 files at r9.
Review status: all files reviewed at latest revision, 15 unresolved discussions.

---

a discussion (no related file):

Previously, pherris (Andrew Ralston) wrote…

I took another swing at tightening up the examples - I'm back to using the basic examples but with new npm run entry points. Let me know what you think... Maybe in future PRs the scope can be smaller so the review process isn't so painful :(

Thanks @pherris! Looking much better. A few more things to do, all pretty minor.

---

examples/basic/package.json, line 16 [r9] (raw file):

    "bs3": "cp -f .bootstraprc-3-example .bootstraprc",
    "bs4": "cp -f .bootstraprc-4-example .bootstraprc",
    "bs3:customlocation": "nodemon --watch app/markup --ext html server.dev.js --bootstraprc-location=.bootstraprc-3-example",

Let's start this with rm .bootstraprc || to be sure the file doesn't exist. Same for all the custom location ones.

---

examples/basic/webpack.dev.config.js, line 12 [r9] (raw file):

  return matchedArgument && matchedArgument.split('=')[1];
}();
var defaultBootstraprcFileExists;

let, not const

---

examples/basic/webpack.dev.config.js, line 24 [r9] (raw file):

  // eslint-disable-next-line no-console
  console.log('This script requires a \'bootstraprc-location\' arg or a ./.boostraprc file in the root.');
  throw new Error('This script requires a \'bootstraprc-location\' arg or a ./.boostraprc file in the root.');

Nice. Is the throw not logged by itself? If so, maybe wrap everything in a try/catch and log the message.

---

examples/basic/webpack.dev.config.js, line 32 [r9] (raw file):

    'bootstrap-loader/lib/bootstrap.loader?',
    `configFilePath=${__dirname}/${bootstraprcCustomLocation}`,
    '!bootstrap-loader/no-op.js'

@alexfedoseev is it possible that that the loader can know that if the file ends in bootstrap.rc, then we don't need to do a no-op.js. The original bootstrap-sass-loader took the loader location as a param.

---

examples/basic/webpack.dev.config.js, line 36 [r9] (raw file):

} else {
  bootstrapEntryPoint.push('bootstrap-loader');
}

let boostrapEntryPoint
if (bootstraprcCustomLocation) {
 bootstrapEntryPoint = 'bootstrap-loader/lib/bootstrap.loader?' + 
    `configFilePath=${__dirname}/${bootstraprcCustomLocation}` +
    '!bootstrap-loader/no-op.js';
   bootstrap
} else {
  bootstrapEntryPoint = 'bootstrap-loader';
}

---

examples/basic/webpack.prod.config.js, line 37 [r9] (raw file):

  ]);
} else {
  bootstrapEntryPoint.push('bootstrap-loader');

Can we avoid the duplication here?

Here's an example of building on an existing config: https://github.com/shakacode/react-webpack-rails-tutorial/blob/master/client/webpack.client.rails.build.config.js#L8

Keep it DRY!

---

node_package/scripts/test, line 9 [r9] (raw file):

# If we want to run tests in the browser
# echo "Running JS tests in browser"
# $(npm bin)/browserify -t babelify node_package/tests/*.js | tape-run | $(npm bin)/faucet

👍 for tests!

---

Comments from Reviewable

[pherris]

Review status: 9 of 13 files reviewed at latest revision, 15 unresolved discussions.

---

examples/basic/package.json, line 16 [r9] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Let's start this with rm .bootstraprc || to be sure the file doesn't exist. Same for all the custom location ones.

I'll do that but I wrote the loader to explicitly look for the custom location first to avoid accidentally using the default .bootstraprc. Also, it's a `&&` not an `||` right?

---

examples/basic/webpack.dev.config.js, line 12 [r9] (raw file):

Previously, justin808 (Justin Gordon) wrote…

let, not const

It's weird, I was using a `let` here but I was getting the error about using `let` and `const` outside of strict mode. I'm not sure what that is about. Any ideas?

---

examples/basic/webpack.dev.config.js, line 24 [r9] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Nice. Is the throw not logged by itself? If so, maybe wrap everything in a try/catch and log the message.

Yeah, probably we can just throw and not log - we don't want to catch because the program should exit if not configured correctly.

---

Comments from Reviewable

[pherris]

Review status: 9 of 13 files reviewed at latest revision, 15 unresolved discussions.

---

examples/basic/webpack.prod.config.js, line 37 [r9] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Can we avoid the duplication here?

Here's an example of building on an existing config: https://github.com/shakacode/react-webpack-rails-tutorial/blob/master/client/webpack.client.rails.build.config.js#L8

Keep it DRY!

Done.

---

Comments from Reviewable

[justin808]

Reviewed 4 of 4 files at r10.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

---

a discussion (no related file):

Previously, justin808 (Justin Gordon) wrote…

Thanks @pherris! Looking much better. A few more things to do, all pretty minor.

Looks GREAT @pherris! Much better!

@alexfedoseev Let's try this out today and release after locally trying.

---

examples/basic/package.json, line 16 [r9] (raw file):

Previously, pherris (Andrew Ralston) wrote…

I'll do that but I wrote the loader to explicitly look for the custom location first to avoid accidentally using the default .bootstraprc. Also, it's a && not an || right?

Its an 'or' BC rm errors if no file

---

Comments from Reviewable

[justin808]

Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

a discussion (no related file):
@pherris I caught one critical issue, and gave a fix in the comments. (IIFE and operator precedence issue).

In the contributing section, please list the exact numbered steps so that future contributors can verify that didn't break these nice examples.

@alexfedoseev should examples/basic/package.json have something like a relative path?

-    "bootstrap-loader": "^1.0.9",
+    "bootstrap-loader": "file://../../bootstrap-loader",

When I use npm link, I got this error:

ERROR in /Users/justin/shakacode/bootstrap-loader/lib/bootstrap.loader.js!/Users/justin/shakacode/bootstrap-loader/no-op.js
Module build failed: TypeError: Cannot read property 'bootstrapVersion' of undefined
    at createConfig (/Users/justin/shakacode/bootstrap-loader/lib/bootstrap.config.js:109:41)
    at Object.module.exports.pitch (/Users/justin/shakacode/bootstrap-loader/lib/bootstrap.loader.js:89:44)
 @ /Users/justin/shakacode/bootstrap-loader/loader.js 1:0-44
webpack: bundle is now VALID.

When I used:

cd examples/basic
npm i --save-dev ../..

That works...

Here's the IIFE error:

➜ ~/shakacode/bootstrap-loader/examples/basic (pherris-master) npm i --save-dev ../..

➜ ~/shakacode/bootstrap-loader/examples/basic (pherris-master) ✗ npm run bs3:dev ✹ [21:50:28]

[email protected] bs3:dev /Users/justin/shakacode/bootstrap-loader/examples/basic
npm run bs3 && npm start

[email protected] bs3 /Users/justin/shakacode/bootstrap-loader/examples/basic
cp -f .bootstraprc-3-example .bootstraprc

[email protected] start /Users/justin/shakacode/bootstrap-loader/examples/basic
nodemon --watch app/markup --ext html server.dev.js

[nodemon] 1.9.1
[nodemon] to restart at any time, enter rs
[nodemon] watching: /Users/justin/shakacode/bootstrap-loader/examples/basic/app/markup/*/
[nodemon] starting node server.dev.js
/Users/justin/shakacode/bootstrap-loader/examples/basic/webpack.bootstrap.config.js:7
}();
^
SyntaxError: Unexpected token (
at Object.exports.runInThisContext (vm.js:76:16)
at Module._compile (module.js:513:28)
at Object.Module._extensions..js (module.js:550:10)
at Module.load (module.js:458:32)
at tryModuleLoad (module.js:417:12)
at Function.Module._load (module.js:409:3)
at Module.require (module.js:468:17)
at require (internal/module.js:20:19)
at Object. (/Users/justin/shakacode/bootstrap-loader/examples/basic/webpack.dev.config.js:7:30)
at Module._compile (module.js:541:32)

---

examples/basic/webpack.bootstrap.config.js, line 7 [r10] (raw file):

  const matchedArgument = process.argv.find(val => val.includes('--bootstraprc-location'));
  return matchedArgument && matchedArgument.split('=')[1];
}();

This crashes due to operator precedence issues.

You can wrap the IIFE with parens, like this:

const bootstraprcCustomLocation = (() => {
  const matchedArgument = process.argv.find(val => val.includes('--bootstraprc-location'));
  return matchedArgument && matchedArgument.split('=')[1];
})();

but why bother?

Why not just have a few extra lines, or just create a function and call it?

An IIFE is ​overkill here.

---

Comments from Reviewable

[justin808]

@pherris I'd like to get this merged soon. There're some other PRs coming after this one. See my comments.

CC: @alexfedoseev

[justin808]

Reviewed 2 of 3 files at r11, 1 of 1 files at r12.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

CONTRIBUTING.md, line 47 [r12] (raw file):

npm install --save-dev ../..
npm run bs4:customlocation

good enough -- I can change this after the merge

We should list all the ones to run.

---

README.md, line 64 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

I think code below is OK, however, put absolute path in bold!

OK

---

examples/basic/.bootstraprc, line 6 [r7] (raw file):

Previously, alexfedoseev (Alex Fedoseev) wrote…

I do like the idea of having the script copy this file from the source and having this exact path in this directory be git ignored. In that case, this file is just deleted.

Sounds good to me 👍

OK

---

examples/basic/package.json, line 19 [r8] (raw file):

Previously, pherris (Andrew Ralston) wrote…

Done.

OK

---

examples/basic/package.json, line 16 [r9] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Its an 'or' BC rm errors if no file

OK

---

examples/basic/package.json, line 54 [r12] (raw file):

    "babel-preset-es2015": "^6.1.18",
    "body-parser": "^1.14.1",
    "bootstrap-loader": "file:///Users/andrew.ralston/Projects/bootstrap-loader",

How about we submit this with "*"?

@vpanjganj, how did you handle this?

Definitely not with the local file path.

---

Comments from Reviewable

[justin808]

@pherris Please fix the reference to your local path, and squash all the commits down to one with a nice commit message. Then we can merge and release and

🎆 🎉

[vpanjganj]

@justin808 In the sample projects:
delete node_module folder
remove bootstrap-loader line from package json
then run this
npm install --save-dev ../../../bootstrap-loader
then do the normal npm i for the rest of dependencies
And need to repeat it each time you make a change in bootstrap-loader

[justin808]

@pherris

1. Please rebase on top of master, as I just merged @vpanjganj 's small change.
2. Let's make sure that @vpanjganj's comment is in the docs for the examples, and maybe the contributing.md.

Be in the correct directory:

cd examples/basic

Then install after each change:

npm install --save-dev ../../../bootstrap-loader

@vpanjganj Once this goes in, we'll cut the v2 beta release.

[vpanjganj]

examples/basic/package.json, line 54 [r12] (raw file):

Previously, justin808 (Justin Gordon) wrote…

How about we submit this with "*"?

@vpanjganj, how did you handle this?

Definitely not with the local file path.

You're right. I changed it form [path to local lib] to "*" . Hoping we will change it to the latest version number after release.

---

Comments from Reviewable

[vpanjganj]

src/bootstrap.loader.js, line 83 [r13] (raw file):

  config.bootstrapPath = resolveModule(bootstrapNPMModule);
  logger.debug(`Bootstrap module location (abs):`, config.bootstrapPath);

Strings must use singlequote

---

Comments from Reviewable

[vpanjganj]

src/bootstrap.loader.js, line 86 [r13] (raw file):

  config.bootstrapRelPath = path.relative(this.context, config.bootstrapPath);
  logger.debug(`Bootstrap module location (rel):`, config.bootstrapRelPath);

Strings must use singlequote

---

Comments from Reviewable

[vpanjganj]

node_package/tests/configCreation.test.js, line 5 [r13] (raw file):

test('userConfigFileExists returns true for files that exist', (assert) => {
  const result = userConfigFileExists(__dirname + '/configCreation.test.js');

Unexpected string concatenation prefer-template

---

Comments from Reviewable

[justin808]

See #114.

CC: @alexfedoseev @pherris @vpanjganj

[justin808]

I've published 1.1.0-beta.1 from #114. I'm awaiting review comments and somebody to verify that the new build works.