-
Notifications
You must be signed in to change notification settings - Fork 445
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
Remove setTheme dynamic require(...) that is problematic with webpack #203
Conversation
@Marak appreciate your review on this PR. What changes if any do you require for merge? |
@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 And, would you mind pulling from Thanks!! |
Setting a theme using a dynamic require is bad practice and causes lots of problems. In case anyone is using that "feature," though, we return a warning that very explicitly tells them the (very simple) change they need to make to their code.
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. |
I'm OK with removing dynamic require statement in The same functionality can be achieved without a dynamic require. |
@Marak Beautiful. That clears the way for us to move this from pre-release to full release. I've published a pre-release, In any case, I believe this RC fixes all of the critical bugs with |
@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. :-( |
Interesting, good catch... I don't know why there's a |
@phjardas Can you please try |
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. 👍 |
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 |
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.