safe mode behaves strangely when using with webpack

[gocreating]

My webpack log keeps showing warning message even when I use the safe mode var colors = require('colors/safe');

The warning message is

WARNING in ./~/colors/lib/colors.js
Critical dependencies:
138:29-43 the request of a dependency is an expression
 @ ./~/colors/lib/colors.js 138:29-43

WARNING in ./~/colors/lib/extendStringPrototype.js
Critical dependencies:
101:31-45 the request of a dependency is an expression
 @ ./~/colors/lib/extendStringPrototype.js 101:31-45

I am not sure whether this problem belongs to webpack or colors...

[pentateu]

I have the same issue.

[vjpr]

colors/lib/colors.js

colors.setTheme = function (theme) {
  if (typeof theme === 'string') {
    try {
      colors.themes[theme] = require(theme); // < ----- Problem.
      applyTheme(colors.themes[theme]);
      return colors.themes[theme];
    } catch (err) {
      console.log(err);
      return err;
    }
  } else {
    applyTheme(theme);
  }
};

[pentateu]

Any solution yet?

[fresheneesz]

setTheme simply shouldn't be using a dynamic require. There is literally no reason to allow object definitions and file paths in this API. Not only is it problematic in the case of webpack, but its confusing since require can use colors/lib as the root for searching for the theme. If the user wants to store a theme in a file, why not let them simply do this?

var theme = require("path/to/themeFile.js")
colors.setTheme(theme)

It makes much more sense.

[fresheneesz]

Then again, since this is a command prompt console library, a fair question is why are you using this module in browsers with webpack?

[gocreating]

@fresheneesz I'm working on a CLI project. The CLI use webpack to bundle another project, and I need to log some information with pretty colors.
Now I use gulp-util instead.

[StephanBijzitter]

@gocreating because colors in a browser console is also useful.
@Marak When will this be fixed? Would you accept a pull request for a new major version that fixes this?

[TaichiHo]

Having the same issues.
Dynamic require is definitely an anti-pattern. Webpack is right to complain about this.

[pedroabreu]

+1

[optimatex]

+1. Pls, tell me some solution

[senaev]

+1

[OmarElabd]

I was able to suppress the message by using this in webpack:

module: {
    exprContextCritical: false,
}

[vjpr]

@OmarElabd That's a bad idea. Critical contexts mean that an unpredicatable number of extra files are being pulled into your project which will bloat your bundle.

[denneulin]

@fresheneesz I use webpack to bundle my project to deploy it on AWS Lambda.

[DualWield]

+1 same issue when use webpack

[haschu]

+1 Same here

[omeid]

@Marak Could you please respond? Do you have the intention of maintaining this project or not? If not, please consider adding contributors or transferring the ownership.

[Marak]

I understand dynamic require isn't best use-case. In most use-cases this block of code isn't being called at all.

I'm glad to see this issue fixed, but don't see one person here who took the time to make pull request or fork project with a fix.

I never tried to use webpack or browserify on colors module. I've tried three times to transfer this project or add core contributors. Each time a separate person broke the build and caused colors to break for my own usage. I still use colors everyday, so I appreciate not seeing it break.

I am individually responsible for a mountain of open-source software across multiple platforms and at this point, colors is a low priority for me. It's a simple piece of software with many viable alternatives. If there is anyone qualified to take the project over email [email protected] with a valid reason why I should trust you.

If not, I'll do my best to try and push out a new version of colors that addresses this issue among with others in a reasonable time-frame.

[omeid]

I really appreciate your response and understand maintaining open source projects can be very time consuming; and adding contributors, or let alone transferring ownership of a project with large number of users isn't something easy.

With that being said, the problematic part mentioned in this issue can be factored out so that importing the theme is the responsibility of the user, as per @fresheneesz's suggestion. Would you be happy to consider a pull request for that change?

[jpap]

Dear @Marak, I've provided a PR for your review. I do hope you're able to consider it for merge as I'm also having a similar webpack warning being issued for my Electron app.

In my case, I am using winston for logging which pulls in your project as a dependency. The warnings emitted from webpack are a double whammy: on the webpack console and in the Electron renderer instance HMR log. The latter is particularly distracting.

[DABH]

Thanks for the PR, I'm going to give it a thorough review. Have to be careful about backwards compatibility etc., but this is a significant enough problem that we might just incorporate this and tag a new major release.

[Marak]

Please make sure that basic require('colors') functionality will always work for * version going back all the way.

[omeid]

@Marak I don't believe it is possible to fix it in a backward compatible way, if people don't care about depending on the correct version of a package, is it right to stop progress for them?

[jpap]

@DABH thanks for taking assignment and review of my PR. As it been sitting idle for nearly five months, and this issue existing way back to 2015, I will make sure to be responsive to your comments so we can finally bring it to a close.

[DABH]

Sure thing. I promised Marak to not break anything, so my number one priority is to tread extremely carefully and test extremely thoroughly before doing anything. I'll try to make a small roadmap doc (if I manage to be so organized), but after some initial issue/PR triaging, it seems like there are:

• a few small issues (fix documentation typos, ignore some files, lint, etc.) that don't possibly affect functionality and generally are "cleanup" items. These look promising for a maintenance release, say 1.2
• a few larger, more important issues like these, that could add or change functionality. If real code is changing, as Marak says, none of the core colors interface must change; all tests must still pass unaltered, etc. However, if there is something totally broken that requires rewriting some internals of the code, let's do it. But we should think hard about ways to not change any existing interfaces. This feels more like a major release, like a 2.0 or something.

In either case, releases will be managed with great care -- e.g. we will do RC releases before publishing any final builds, etc.

Thanks for your responsiveness -- I will try to return the favor! Don't expect too much, but as a first guess, I'll attempt to have a review done on this within a week or two. Feel free to bother me until we reach a conclusion :)

FWIW I don't think this issue is part of the core API, and the "bad" overload of this function feels like an anti-pattern anyway, so I think we will be able to find a positive resolution here... but stay tuned... :)

[omeid]

@DABH Thanks for taking this on.

[DABH]

Please check out colors@next and the readme for the repo! If you can, use the pre-release version in your projects and open issues if you find anything... this should, at long last, be resolved!