Update themr.js

[idangozlan]

when the source style is empty / contain only :global rules, extract-text-webpack-plugin put string instead of json, which break the merge. this if prevent this specific case https://github.com/javivelasco/react-css-themr/issues/66

[raveclassic]

Thanks for PR!

Still I don't think that it is a solution because then themr starts to depend on some magic string from the other library. When it changes and no doubt one day it changes, we'll get the same error so I think we need to find more generic solution.

On the other hand, doesn't extract-plugin violate the principe of a css-module? Shouldn't an import of a module always be an object? Shouldn't it be an empty object with no keys when there are no styles? Could you please check what does an import contain without extract plugin for a file with only global styles?

UPDATE: Moreover why would you apply an empty theme with no keys anyway?

[idangozlan]

It's giving the string of removed by when it's only globals. Idk if it's violate it but it makes sense.

On Jun 8, 2017 10:31, "Kirill Agalakov" notifications@github.com wrote:

Thanks for PR!

Still I don't think that it is a solution because then themr starts to depend on some magic string from the other library. When it changes and no doubt one day it changes, we'll get the same error so I think we need to find more generic solution.

On the other hand, doesn't extract-plugin violate the principe of a css-module? Shouldn't an import of a module always be an object? Shouldn't it be an empty object with no keys when there are no styles? Could you please check what does an import contain without extract plugin for a file with only global styles?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/javivelasco/react-css-themr/pull/68#issuecomment-307023282, or mute the thread https://github.com/notifications/unsubscribe-auth/AB5hbYP7HFL_POZ5PaJCc74kIOAeCJ7Wks5sB6NOgaJpZM4NzSC9 .

[raveclassic]

So as far as I can see, extract plugin changes the type of imported module from object to string when all styles are global, am I right? Then can we check module's type when passing it to decorator and throw the error earlier? Or even do not merge anything and take theme object from props as is?

[idangozlan]

You right. About the merge, right now the only thing that prevent it to be merged is the error throwing. We can transit it to warning without throwing exception.. what do you think?

On Jun 8, 2017 10:46, "Kirill Agalakov" notifications@github.com wrote:

So as far as I can see, extract plugin changes the type of imported module from object to string when all styles are global, am I right? Then can we check module's type when passing it to decorator and throw the error earlier? Or even do not merge anything and take theme object from props as is?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/javivelasco/react-css-themr/pull/68#issuecomment-307026370, or mute the thread https://github.com/notifications/unsubscribe-auth/AB5hbe5qr6l8yrDMzl0YSj7IpfVxRjXCks5sB6bggaJpZM4NzSC9 .

[raveclassic]

But it's incorrect to try to merge a string with an object and suppressing the error or replacing it with a warning is more like an in-place hack.

Maybe the best solution is not to apply an empty theme at all? Could you provide more concrete example of your component? Do you specify a shape of the theme either via propTypes or ts/flow type/interface? If you do then then the problem is that you pass incorrect shape - a string. And this string is produced by third-party library (extract-plugin) so why should themr adjust its behavior to cover such edge-case which is absolutely out of scope of applying a theme to a component? Furthermore I think that we should also throw an error when passing incorrect theme to @themr decorator as default theme to fail earlier.

Here's an example of type-related solution:

import * as theme from './only-globals.css'; //theme === //removed by extract-plugin
import { themr } from 'react-css-themr';

type TProps = {
  theme: { //you specify a strict API which should be respected
    container: string
  }
}

//do not apply default theme because it's actually empty and is not assignable to defined type
//we should also throw here in runtime
@themr(Symbol()/*, theme*/) 
export class Foo extends React.Component<TProps, never> {
}

[idangozlan]

That's exactly what I've done, but it took me long time to understand what was the error.

Maybe we can create a warning when the string is the specific one of the extract plugin, to help users solve that quickly.

On Jun 8, 2017 13:31, "Kirill Agalakov" notifications@github.com wrote:

But it's incorrect to try to merge a string with an object and suppressing the error or replacing it with a warning is more like an in-place hack.

Maybe the best solution is not to apply an empty theme at all? Could you provide more concrete example of your component? Do you specify a shape of the theme either via props or ts/flow type/interface? If you do then then the problem is that you pass incorrect shape - a string. And this string is produced by third-party library (extract-plugin) so why should themr adjust its behavior to cover such edge-case which is absolutely out of scope of applying a theme to a component? Furthermore I think that we should also throw an error when passing incorrect theme to @themr decorator as default theme to fail earlier.

Here's an example of type-related solution:

import as theme from './only-globals.css'; //theme === //removed by extract-pluginimport { themr } from 'react-css-themr'; type TProps = { theme: { //you specify a strict API which should be respected container: string } } //do not apply default theme because it's actually empty and is not assignable to defined type//we should also throw here in runtime @themr(Symbol()/, theme*/) export class Foo extends React.Component<TProps, never> { }

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/javivelasco/react-css-themr/pull/68#issuecomment-307065068, or mute the thread https://github.com/notifications/unsubscribe-auth/AB5hbez80OVx2jJZ4ywp8BesRPnkTu7fks5sB82bgaJpZM4NzSC9 .

[raveclassic]

I still don't think that hardcoding a string // removed by... is a good idea. Adding a check for default theme type in the beginning of @themr decorator would emit an error in design-time instead of run-time and I think it would be enough for a developer to understand that a theme passed to decorator is invalid. What do you think?

[idangozlan]

maybe we should modify the current error to more informative one, that also tell about possible case of having empty base stylesheet.

Do you want me to pull request it?

On Thu, Jun 8, 2017 at 2:39 PM, Kirill Agalakov notifications@github.com wrote:

I still don't think that hardcoding a string // removed by... is a good idea. Adding a check for default theme type in the beginning of @themr decorator would emit an error in design-time instead of run-time and I think it would be enough for a developer to understand that a theme passed to decorator is invalid. What do you think?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/javivelasco/react-css-themr/pull/68#issuecomment-307078502, or mute the thread https://github.com/notifications/unsubscribe-auth/AB5hbYHj2jXpPkKDETgmg4DCTsyQmpb5ks5sB919gaJpZM4NzSC9 .

[raveclassic]

Well, errors in themeable function imo should be related only to type incompatibilities while I agree that @themr decorator could throw something more informative.

Would you mind updating the PR?

[idangozlan]

Yep. Thanks for your attention and quick responses.

On Thu, Jun 8, 2017 at 2:51 PM, Kirill Agalakov notifications@github.com wrote:

Well, errors in themeable function imo should be related only to type incompatibilities while I agree that @themr decorator could throw something more informative.

Would you mind updating the PR?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/javivelasco/react-css-themr/pull/68#issuecomment-307080823, or mute the thread https://github.com/notifications/unsubscribe-auth/AB5hbcjdrbzVrF5dRWQzr3BZgXGNTQ_2ks5sB-BHgaJpZM4NzSC9 .

[idangozlan]

Done.

[raveclassic]

Great, thanks! Now we need @javivelasco to merge it