Remove setTheme dynamic require(...) that is problematic with webpack

[jpap]

Addresses #200, #137.

The setTheme method is refactored to remove the ability to import directly from a file; it is now the caller's responsibility to create a theme object to pass as the single argument. The example showing the old behavior has been refactored to suit.

I chose to refactor in this way because the method of setting the theme from a file was not documented in the README and it appears not be a usual use-case in general.

[jpap]

@marak appreciate your review on this PR. What changes if any do you require for merge?

[DABH]

@jpap or others -- so there is no way to dynamically require a module, and have webpack not complain? I've done some investigation and that seems to be the case, but it's surprising there's no workaround...

Also, for this PR, we should print a warning if typeof theme === string (right now it just silently fails). Just in case someone was using this bad design (https://xkcd.com/1172/). Could you add something like that? (Tell them exactly what usage is wrong, and give explicitly the correct syntax... "add require(...) where you're calling setTheme..." or something.)

And, would you mind pulling from origin/develop, so CI tests will pass again?

Thanks!!

[DABH]

I did this myself. I'm going to merge this in for now, and then conduct additional testing. I plan to publish a pre-release version (colors@next) soon, so everyone can start testing this in their own use cases to see if there's anything we missed.

[Marak]

I'm OK with removing dynamic require statement in colors.

The same functionality can be achieved without a dynamic require.

[DABH]

@Marak Beautiful. That clears the way for us to move this from pre-release to full release.

I've published a pre-release, 1.2.0-rc0, and added a note to the readme (https://github.com/Marak/colors.js#colorsjs-) directing people towards this version. It's a pre-release, so it will not install by default, but if you explicitly ask for it you'll get it. My plan is to let this version sit for a week or a couple weeks, before releasing another RC or publishing 1.2.0. (@Marak if you want things published sooner, let me know or feel free). Ideally we could get some users adopting the RC, testing this in the wild across a bunch of platforms, so we feel more confident about fully releasing this version.

In any case, I believe this RC fixes all of the critical bugs with colors, so this should make users happy for a long time to come :)

[phjardas]

@DABH @Marak I think the bug is only partially fixed. The same code is still present in https://github.com/Marak/colors.js/blob/master/lib/extendStringPrototype.js#L95 and still breaks my Webpack build. :-(

[DABH]

Interesting, good catch... I don't know why there's a setTheme in that file as well?? Likely as not that should just be deleted, or at least the same fix applied there. If everything works without that seemingly repetitive function there though... let's delete it.

[DABH]

@phjardas Can you please try colors@1.3.0 and let me know if it works for you now? That setTheme is definitely needed, so I just applied the same patch as in 5c84a86 to this version of the function. All tests pass for me and everything's looking good in Node, but haven't tested in Webpack. Would be great if it works though, so do let me know!

[phjardas]

Verified: https://gist.github.com/phjardas/4c7b26536c9d7c151a8913e7d1137df2

The build with 1.2.0 emits the known warning, the build with 1.3.0 does not. :+1:

[DABH]

Great! Thanks for the gist! I'll think about whether we can add something like that to the examples/docs just to show off the fact that colors should work with webpack now. :+1: