feat: support for receiving a `{Function}` (`options.modifyVars`) #230

stackjie · 2017-11-16T13:37:01Z

No description provided.

jsf-clabot · 2017-11-16T13:37:02Z

All committers have signed the CLA.

codecov · 2017-11-16T13:42:58Z

Codecov Report

Merging #230 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   97.84%   97.89%   +0.04%     
==========================================
  Files           8        8              
  Lines          93       95       +2     
  Branches        9       10       +1     
==========================================
+ Hits           91       93       +2     
  Misses          2        2

Impacted Files	Coverage Δ
src/getOptions.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acb91d2...c1c3fcc. Read the comment docs.

michael-ciniawsky

👍 So far :)

michael-ciniawsky · 2017-11-16T16:26:05Z

src/getOptions.js

@@ -34,6 +34,10 @@ function getOptions(loaderContext) {
    }
  }



if (typeof options.modifyVars === 'string') { try { loaderContext.addDependency(options.modifyVars); // Load variables from external file options.modifyVars = require(options.modifyVars); } catch (err) { loaderContext.emitError(new Error(`Less Loader (modifyVars)\n\n${err.message}`)); } }

require will be cached and the reload will not work

delete require.cache[options.modifyVars] (before ... = require(options.modifyVars))

if (typeof options.modifyVars === 'string') { try { loaderContext.addDependency(options.modifyVars); // Clean the variables cache (require.cache) delete require.cache(options.modifyVars); // Load variables from external file options.modifyVars = require(options.modifyVars); } catch (err) { loaderContext.emitError(new Error(`Less Loader (modifyVars)\n\n${err.message}`)); } }

If you don't want to include this addition in your PR it's also fine to leave as is :)
It's just a suggestion for support of modifyVars: path.resolve(__dirname, 'path/to/vars.json') {String} in webpack.config.js

good suggestion😀

michael-ciniawsky · 2017-11-16T16:28:14Z

test/index.test.js

+  let res = false;
+  const loaderOptions = {
+    modifyVars: (loader) => {
+      res = typeof loader === 'object';


Could this actually test a small .less file with some vars ? Can you confirm it works correctly in a real world project ?

stackjie · 2017-11-17T04:53:12Z

@michael-ciniawsky I updated the test method and made an example repo.
less-loader-modifyvars-example
In my real project has been applied.

michael-ciniawsky · 2017-11-17T16:36:30Z

test/index.test.js

+    },
+  };
+
+  let inspect;


michael-ciniawsky · 2017-11-17T16:36:43Z

test/index.test.js

+test('should support receiving a function for options.modifyVars', async () => {
+  const loaderOptions = {
+    modifyVars: (loader) => {
+      expect(loader).toBeInstanceOf(Object);


michael-ciniawsky · 2017-11-17T16:37:02Z

test/index.test.js

+  });
+
+  await compile('options-modifyvars-function', rules)
+      .catch(e => e);


e => err && \n

michael-ciniawsky · 2017-11-17T16:37:14Z

test/index.test.js

+
+  await compile('options-modifyvars-function', rules)
+      .catch(e => e);
+  const [css] = inspect.arguments;


joshwiens

Review comments aside, features for both Sass & Less loaders have to be approved by @jhnns per our agreement prior to him starting work on his dissertation

tim-soft · 2017-12-02T00:59:30Z

Does this PR need more modifications? I'm very keen on trying out this feature

benperiton · 2018-02-28T11:16:16Z

Is this likely to land soon?
Would be very useful.

jhnns

@stackjie Thanks for your PR 🎉

Can you elaborate a use case where this feature is useful? Why is the current option not enough?

Sorry for the delay btw 😞

jhnns · 2018-04-10T10:47:28Z

src/getOptions.js

@@ -34,6 +34,10 @@ function getOptions(loaderContext) {
    }
  }

+  if (typeof options.modifyVars === 'function') {
+    options.modifyVars = options.modifyVars(loaderContext);


What's the idea behind passing the loaderContext to modifyVars? I'm trying to avoid these kind of extra features that make it hard to migrate/change anything later since we already exposed it to the outside.

stackjie · 2018-04-14T01:31:20Z

@jhnns issues #229
I want to hot reload webpack when changing values in modifyvars, Or is there a better way?
thanks😄

jhnns · 2018-04-16T14:36:27Z

So you basically want to hot reload upon config changes which is really hard/almost impossible to do in a clean way. This is also not possible with other parts of the webpack.config.js, so I guess it's just consistent to not support here as well. I think you can achieve a similar feature with nodemon. Add this to your package.json scripts:

    "scripts": {
        "build-watch": "nodemon npx webpack --config path/to/config"
    }

This restarts the process as soon as you change the config. Not exactly hot reloading, but still some kind of watch mechanism.

Does that help you?

stackjie · 2018-04-16T14:56:18Z

@jhnns Thank you so much for your suggestion, my problem has been solved, this PR can be closed.😁

ndbroadbent · 2018-12-08T15:18:01Z

I'm working on a plugin to set up Ant Design for create-react-app, and I've been trying to see if I can solve this issue about auto-reloading the modifyVars option for webpack-dev-server. I started looking at fs.watch to modify the object when the file was changed, but then I found this PR, which is a much better solution.

I'm loading the vars from a Less or JSON file, so it would be awesome if I could force webpack-dev-server to recompile whenever there's a change. I don't really like the nodemon workaround.

I've just tried this PR, and I'm able to pass modifyVars as a function that loads the vars from a file.

$ FORCE_COLOR=true npm start | cat

> myapp@0.1.0 start code/myapp
> craco start

Starting the development server...

[DEBUG] ====> Loading modifyVars from src/style/AntDesign/customTheme.less
[DEBUG] ====> modifyVars:  {"@primary-color":"#b455d9","@link-color":"#2639dd","@border-radius-base":"5px"}
...  // Lots of the same output
Compiled successfully!
...
Compiling...
[DEBUG] ====> Loading modifyVars from src/style/AntDesign/customTheme.less
[DEBUG] ====> modifyVars:  {"@primary-color":"#b455d9","@link-color":"#1aa760","@border-radius-base":"5px"}
Compiled successfully!

Changing this customTheme.less file doesn't trigger a recompile, so I had to change a different Less file to trigger the recompile. (And you can see that @link-color was updated in the logs.) Unfortunately this doesn't actually change the color in my browser, so I probably need a way to clear the cache and recompile everything. It would be nice if there was a hook I could use, so I could use fs.watch to watch the file and then trigger a full recompile with the new modifyVars option.

This is also not possible with other parts of the webpack.config.js, so I guess it's just consistent to not support here as well.

That's a good point, but it would be really nice if it was possible. It doesn't seem too difficult in theory, and this PR would make it a bit easier.

I think I'm at a dead-end though, so I'll just add the nodemon suggestion to the README for now. But wanted to leave a comment to show how this might be used. If it's possible to force a recompile in a future version of webpack, then this PR would be really useful.

feat: support receiving a function for options modifyVars (webpack-co…

aa8b776

…ntrib#229)

michael-ciniawsky reviewed Nov 16, 2017

View reviewed changes

michael-ciniawsky added priority: 4 (important) pr_action: Review semver: Minor status: Pending type: Feature labels Nov 16, 2017

michael-ciniawsky requested review from jhnns, alexander-akait and joshwiens November 16, 2017 16:29

michael-ciniawsky added this to the 4.1.0 milestone Nov 16, 2017

michael-ciniawsky added the flag: Needs Docs label Nov 16, 2017

michael-ciniawsky changed the title ~~feat: support receiving a function for options modifyVars (#229)~~ feat: support for receiving a {Function} (options.modifyVars) Nov 16, 2017

test: update options.modifyVars test

c1c3fcc

michael-ciniawsky reviewed Nov 17, 2017

View reviewed changes

joshwiens reviewed Nov 19, 2017

View reviewed changes

jhnns reviewed Apr 10, 2018

View reviewed changes

jhnns mentioned this pull request Apr 17, 2018

feat: pass loaderContext to custom importer webpack-contrib/sass-loader#513

Closed

ndbroadbent mentioned this pull request Dec 8, 2018

recompile after editing .less files? DocSpring/craco-antd#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support for receiving a `{Function}` (`options.modifyVars`) #230

feat: support for receiving a `{Function}` (`options.modifyVars`) #230

stackjie commented Nov 16, 2017

jsf-clabot commented Nov 16, 2017 •

edited

Loading

codecov bot commented Nov 16, 2017 •

edited

Loading

michael-ciniawsky left a comment

michael-ciniawsky Nov 16, 2017 •

edited

Loading

stackjie Nov 17, 2017

michael-ciniawsky Nov 17, 2017 •

edited

Loading

michael-ciniawsky Nov 17, 2017

michael-ciniawsky Nov 17, 2017 •

edited

Loading

stackjie Nov 17, 2017

michael-ciniawsky Nov 16, 2017

stackjie commented Nov 17, 2017 •

edited

Loading

michael-ciniawsky Nov 17, 2017

michael-ciniawsky Nov 17, 2017

michael-ciniawsky Nov 17, 2017 •

edited

Loading

michael-ciniawsky Nov 17, 2017

joshwiens left a comment

tim-soft commented Dec 2, 2017

benperiton commented Feb 28, 2018

jhnns left a comment

jhnns Apr 10, 2018

stackjie commented Apr 14, 2018

jhnns commented Apr 16, 2018

stackjie commented Apr 16, 2018

ndbroadbent commented Dec 8, 2018 •

edited

Loading

feat: support for receiving a {Function} (options.modifyVars) #230

feat: support for receiving a {Function} (options.modifyVars) #230

Conversation

stackjie commented Nov 16, 2017

jsf-clabot commented Nov 16, 2017 • edited Loading

codecov bot commented Nov 16, 2017 • edited Loading

Codecov Report

michael-ciniawsky left a comment

Choose a reason for hiding this comment

michael-ciniawsky Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackjie commented Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshwiens left a comment

Choose a reason for hiding this comment

tim-soft commented Dec 2, 2017

benperiton commented Feb 28, 2018

jhnns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackjie commented Apr 14, 2018

jhnns commented Apr 16, 2018

stackjie commented Apr 16, 2018

ndbroadbent commented Dec 8, 2018 • edited Loading

feat: support for receiving a `{Function}` (`options.modifyVars`) #230

feat: support for receiving a `{Function}` (`options.modifyVars`) #230

jsf-clabot commented Nov 16, 2017 •

edited

Loading

codecov bot commented Nov 16, 2017 •

edited

Loading

michael-ciniawsky Nov 16, 2017 •

edited

Loading

michael-ciniawsky Nov 17, 2017 •

edited

Loading

michael-ciniawsky Nov 17, 2017 •

edited

Loading

stackjie commented Nov 17, 2017 •

edited

Loading

michael-ciniawsky Nov 17, 2017 •

edited

Loading

ndbroadbent commented Dec 8, 2018 •

edited

Loading