Skip to content
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

Allow styleLoaders to depends to env #222

Merged
merged 9 commits into from
Dec 7, 2016

Conversation

bertho-zero
Copy link
Contributor

@bertho-zero bertho-zero commented Dec 4, 2016

"env": {
    "development": {
      "styleLoaders": ["style-loader", "css-loader", "resolve-url-loader", "sass-loader"],
      "extractStyles": false
    },
    "production": {
      "styleLoaders": ["style-loader", "css-loader", "sass-loader"],
      "extractStyles": true
    }
  }

This change is Reviewable

```
"env": {
    "development": {
      "styleLoaders": ["style-loader", "css-loader", "resolve-url-loader", "sass-loader"],
      "extractStyles": false
    },
    "production": {
      "styleLoaders": ["style-loader", "css-loader", "sass-loader"],
      "extractStyles": true
    }
  }
```
@justin808
Copy link
Member

@bertho-zero:

  1. Why this param only?
  2. What's the use case?
  3. Can you provide a test for this?

If we're going to do this, we'd need a readme note on this, as well as a changelog entry.

@bertho-zero
Copy link
Contributor Author

bertho-zero commented Dec 4, 2016

There is already the extractStyles parameter, and it is the only ones, with styleLoaders, that can change between development and production.

My use case, this avoids having to create 2 files:

"env": {
    "development": {
      "styleLoaders": ["style-loader", "css-loader", "resolve-url-loader", "sass-loader"],
      "extractStyles": false
    },
    "production": {
      "styleLoaders": ["style-loader", "css-loader", "sass-loader"],
      "extractStyles": true
    }
  }

I do not think there's any need for additional testing.

I updated version, changelog and readme.

@justin808
Copy link
Member

This looks good with one tiny thing.

@Judahmeek, do you concur?


Reviewed 2 of 4 files at r2, 1 of 1 files at r4.
Review status: 3 of 4 files reviewed at latest revision, 1 unresolved discussion.


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

## [2.0.0.beta.17] - 2016-12-04
##### Added
- Allow `styleLoaders` to depend on the environment variable 'NODE_PATH' [#222](https://github.com/shakacode/bootstrap-loader/pull/222) by [bertho-zero](https://github.com/bertho-zero).

@bertho-zero You missed the change at the bottom of the file.


Comments from Reviewable

@justin808
Copy link
Member

Reviewed 1 of 4 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@bertho-zero
Copy link
Contributor Author

It is done

@Judahmeek
Copy link
Contributor

Looks good to me.

@justin808
Copy link
Member

:lgtm:


Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@justin808
Copy link
Member

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


README.md, line 224 at r5 (raw file):

  - sass-loader

# You can apply loader params here:

@bertho-zero Can you do me a favor and create 3 different fenced code blocks with a short description of each technique.

It's not appropriate to put this all in one code block as if you'd do this all in the same file.


Comments from Reviewable

@justin808
Copy link
Member

@bertho-zero Please update one thing in the README.md


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


Comments from Reviewable

@bertho-zero
Copy link
Contributor Author

I separated the example of other, yes it was confused.

On the other hand I do not see what examples I could add..

Not speaking very good English, I do not know what to add to the readme.

@justin808 justin808 merged commit 09515c5 into shakacode:master Dec 7, 2016
justin808 pushed a commit that referenced this pull request Dec 9, 2016
Allow styleLoaders to depend on env

```
"env": {
    "development": {
      "styleLoaders": ["style-loader", "css-loader", "resolve-url-loader", "sass-loader"],
      "extractStyles": false
    },
    "production": {
      "styleLoaders": ["style-loader", "css-loader", "sass-loader"],
      "extractStyles": true
    }
  }
```
* Update bootstrap.config.js
* Update package.json
* support styleLoaders env with userConfig
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants