make bootstrapPath configurable through query string

[vjpr]

#254

---

This change is 

[justin808]

@vjpr this looks good. @alexfedoseev will have to review.

Please include in this PR the README.md and changelog entry.

Also, would this apply to the loader v1 and the upcoming v2 which supports only webpack?

@alexfedoseev I'd like to see us soon stop adding any feature/capabilities to the v1 branch.

[alex35mil]

What is shrinkwrap.yaml? The rest LGTM.

[alex35mil]

I'd like to see us soon stop adding any feature/capabilities to the v1 branch.

I don't think we should be doing anything except hotfixes for v1 as webpack v2 is released as stable.

[justin808]

@vjpr Please update to master. That might fix the CI issue! thanks to @Judahmeek!

[vjpr]

@alexfedoseev Oops, shrinkwrap.yaml slipped in there from pnpm. Removed.

@justin808: Also, would this apply to the loader v1 and the upcoming v2 which supports only webpack?

It should work fine with loader v1, but its probably not worth back porting.

---

• Have modified the error message when bootstrap not found.
• Added instructions to FAQ section in README.md.

---

Just need to fix some lint errors.

[justin808]

Some suggestions! Looking awesome!

---

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

---

CHANGELOG.md, line 8 at r4 (raw file):

## Unreleased
##### Added
- Make bootstrapPath configurable. [#255](https://github.com/shakacode/bootstrap-loader/pull/255).

Make bootstrapPath configurable to allow XXXXX

and see other ones for examples. Take credit!

---

README.md, line 537 at r4 (raw file):

### Using a custom location for bootstrap module

By default, `bootstrap-loader` will try to resolve `bootstrap` from where `bootstrap-loader` has been installed. In [certain situations](https://github.com/shakacode/bootstrap-loader/issues/254) (e.g. npm linking, using a custom package installer) it may not be resolvable. In this case, you can pass in the location manually.

Please update https://github.com/shakacode/bootstrap-loader/blob/master/CONTRIBUTING.md with regards to how we can use link.

Also, any need to update the examples?

CC: @Judahmeek

---

node_package/tests/utils/buildExtractStylesLoader.test.js, line 25 at r4 (raw file):

    path.join(
      // eslint-disable-next-line prefer-template
      fs.realpathSync(path.join(__dirname, '../../../node_modules/extract-text-webpack-plugin')) +

see a few lines up to avoid the eslint disable

---

src/bootstrap.loader.js, line 58 at r4 (raw file):

  if (this.cacheable) this.cacheable();

  const { extractStyles, configFilePath, bootstrapPath } = loaderUtils.parseQuery(this.query);

should we allow passing bootstrapVersion?

---

src/bootstrap.loader.js, line 101 at r4 (raw file):

  logger.debug('Query from webpack config:', this.query || '*none*');

  const bootstrapVersion = config.bootstrapVersion || 4;

@vjpr @alexfedoseev @Judahmeek This is potentially a big change. Why is this needed?

I'm not a fan of changing the default to v4 until: https://v4-alpha.getbootstrap.com/ changes from alpha to released. Thoughts?

How about allowing the bootstrap version in the query? or reading it from the directory we get from the bootstrap path?

---

src/bootstrap.loader.js, line 113 at r4 (raw file):

  logger.debug(`Bootstrap module location (abs): ${config.bootstrapPath}`);
  if (!config.bootstrapPath) {
    /* eslint-disable quotes */

see tests for how to work around this string thing...

---

Comments from Reviewable

[alex35mil]

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

---

src/bootstrap.loader.js, line 58 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

should we allow passing bootstrapVersion?

We have bootstrapVersion setting in config from the very beginning.

---

src/bootstrap.loader.js, line 101 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@vjpr @alexfedoseev @Judahmeek This is potentially a big change. Why is this needed?

I'm not a fan of changing the default to v4 until: https://v4-alpha.getbootstrap.com/ changes from alpha to released. Thoughts?

How about allowing the bootstrap version in the query? or reading it from the directory we get from the bootstrap path?

We already have default version set: https://github.com/shakacode/bootstrap-loader/blob/master/src/bootstrap.config.js#L13

---

Comments from Reviewable

[justin808]

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

---

src/bootstrap.loader.js, line 101 at r4 (raw file):

Previously, alexfedoseev (Alex Fedoseev) wrote…

We already have default version set: https://github.com/shakacode/bootstrap-loader/blob/master/src/bootstrap.config.js#L13

So the || 4 should be removed?

---

Comments from Reviewable

[alex35mil]

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

---

src/bootstrap.loader.js, line 101 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

So the || 4 should be removed?

Yes, or DEFAULT_VERSION should be used instead.

---

Comments from Reviewable

[justin808]

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

---

src/bootstrap.loader.js, line 101 at r4 (raw file):

Previously, alexfedoseev (Alex Fedoseev) wrote…

Yes, or DEFAULT_VERSION should be used instead.

@vjpr Can you please make this change and we can get this merged.

---

Comments from Reviewable

[justin808]

@vjpr We're waiting on a tiny change from you so we can merge this and create a release.

---

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

---

Comments from Reviewable

[vjpr]

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

---

CHANGELOG.md, line 8 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Make bootstrapPath configurable to allow XXXXX

and see other ones for examples. Take credit!

Done.

---

README.md, line 537 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Please update https://github.com/shakacode/bootstrap-loader/blob/master/CONTRIBUTING.md with regards to how we can use link.

Also, any need to update the examples?

CC: @Judahmeek

I would leave the testing setup as it is for now. This PR doesn't break it, and its for a different use case than testing with local examples.

---

node_package/tests/utils/buildExtractStylesLoader.test.js, line 25 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

see a few lines up to avoid the eslint disable

Why should we prefer template? path.join makes more sense as it will handle the slashes correctly, and it will work cross platform I think.

---

src/bootstrap.loader.js, line 58 at r4 (raw file):

Previously, alexfedoseev (Alex Fedoseev) wrote…

We have bootstrapVersion setting in config from the very beginning.

But if you wanted to pass it in as a query param. I think it is a good idea for another PR.

---

src/bootstrap.loader.js, line 101 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@vjpr Can you please make this change and we can get this merged.

DEFAULT_VERSION is not available in bootstrap.loader.js. It looks like its set in bootstrap.config.js so I will just remove it. I think it was accidentally added.

---

src/bootstrap.loader.js, line 113 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

see tests for how to work around this string thing...

Just seemed less clean to escape single quotes which seem standard for quoting terms within strings. I will add the escapes though.

---

Comments from Reviewable

[justin808]

BUT please revise to work within our linter guidelines.

---

Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

node_package/tests/utils/buildExtractStylesLoader.test.js, line 25 at r4 (raw file):

Previously, vjpr (Vaughan Rouesnel) wrote…

Why should we prefer template? path.join makes more sense as it will handle the slashes correctly, and it will work cross platform I think.

@vjpr it's not about path.join. It's about doing string concatenation with a + when you could use an ES6 template. See the first test in this file.

---

src/bootstrap.loader.js, line 58 at r4 (raw file):

Previously, vjpr (Vaughan Rouesnel) wrote…

But if you wanted to pass it in as a query param. I think it is a good idea for another PR.

I'm curious if the bootstrapPath can be in the configFile? Maybe the idea is that you don't need a config file?

If you did pass in the bootstrapPath, then we should be checking the bootstrap version within that path.

---

src/bootstrap.loader.js, line 118 at r5 (raw file):

    msg += 'See https://github.com/shakacode/bootstrap-loader/blob/master/README.md#usage.';
    throw new Error(msg);
    /* eslint-enable quotes */

Please use the ES6 template format for this string and remove the eslint-enable quotes.

---

Comments from Reviewable

[justin808]

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

---

src/bootstrap.loader.js, line 113 at r4 (raw file):

Previously, vjpr (Vaughan Rouesnel) wrote…

Just seemed less clean to escape single quotes which seem standard for quoting terms within strings. I will add the escapes though.

Please update for the linter.

---

Comments from Reviewable

[vjpr]

Review status: 3 of 4 files reviewed at latest revision, 5 unresolved discussions.

---

node_package/tests/utils/buildExtractStylesLoader.test.js, line 25 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@vjpr it's not about path.join. It's about doing string concatenation with a + when you could use an ES6 template. See the first test in this file.

Done.

---

src/bootstrap.loader.js, line 58 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I'm curious if the bootstrapPath can be in the configFile? Maybe the idea is that you don't need a config file?

If you did pass in the bootstrapPath, then we should be checking the bootstrap version within that path.

So for me, the bootstrap path needs to be set dynamically at build time depending on whether bootstrap-loader is npm linked or not, and depending on which package installer I am using.

So for this use case, having it inside the config file would be useless.

You you are right, if the bootstrapPath is being set, it would be great to check the version of the package automatically.

---

src/bootstrap.loader.js, line 113 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Please update for the linter.

Done.

---

src/bootstrap.loader.js, line 118 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Please use the ES6 template format for this string and remove the eslint-enable quotes.

Oops, accidentally left that in there.

---

Comments from Reviewable

[justin808]

one question...

---

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

node_package/tests/utils/buildExtractStylesLoader.test.js, line 16 at r6 (raw file):

test('buildExtractStylesLoader runs as expected', (assert) => {
  assert.equals(buildExtractStylesLoader(['style-loader', 'url-loader', 'css-loader']),
    path.join(

if there is only one parameter, why do we use path.join?

---

node_package/tests/utils/buildExtractStylesLoader.test.js, line 21 at r6 (raw file):

  );
  assert.equals(buildExtractStylesLoader(['isomorphic-style-loader', 'url-loader', 'css-loader']),
    path.join(

if there is only one parameter, why do we use path.join?

---

src/bootstrap.loader.js, line 118 at r5 (raw file):

Previously, vjpr (Vaughan Rouesnel) wrote…

Oops, accidentally left that in there.

please change this msg to be a const and use the ES6 template syntax. You won't need to escape the single quotes since you're wrapping in back ticks.

---

Comments from Reviewable

[vjpr]

Review status: 2 of 4 files reviewed at latest revision, 5 unresolved discussions.

---

node_package/tests/utils/buildExtractStylesLoader.test.js, line 16 at r6 (raw file):

Previously, justin808 (Justin Gordon) wrote…

if there is only one parameter, why do we use path.join?

Oops.

---

src/bootstrap.loader.js, line 118 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

please change this msg to be a const and use the ES6 template syntax. You won't need to escape the single quotes since you're wrapping in back ticks.

Done.

---

Comments from Reviewable

[vjpr]

Review status: 2 of 4 files reviewed at latest revision, 5 unresolved discussions.

---

node_package/tests/utils/buildExtractStylesLoader.test.js, line 21 at r6 (raw file):

Previously, justin808 (Justin Gordon) wrote…

if there is only one parameter, why do we use path.join?

Done.

---

Comments from Reviewable

[justin808]

@vjpr

But we need to rebase this on top of master. Let me know if you can do that.

Also, I'm guessing we don't need this for the v1 branch.

@Judahmeek @alexfedoseev Can we release 2.0.0 with this change?

---

Reviewed 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

---

Comments from Reviewable

[justin808]

@vjpr Please let me know when you've resolved the conflict.

[justin808]

See 2857d2b

[justin808]

Merged

[justin808]

2.0.0-beta.22

[justin808]

@vjpr Please test this out ASAP in case there are issues! I had to resolve some merge conflicts.