Update themr.js #68
Update themr.js #68
Conversation
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 javivelasco#66
|
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: |
|
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" <[email protected]> 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
<#68 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB5hbYP7HFL_POZ5PaJCc74kIOAeCJ7Wks5sB6NOgaJpZM4NzSC9>
.
|
|
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? |
|
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" <[email protected]> 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
<#68 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB5hbe5qr6l8yrDMzl0YSj7IpfVxRjXCks5sB6bggaJpZM4NzSC9>
.
|
|
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? 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> {
} |
|
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" ***@***.***> 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
<#68 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB5hbez80OVx2jJZ4ywp8BesRPnkTu7fks5sB82bgaJpZM4NzSC9>
.
|
|
I still don't think that hardcoding a string |
|
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 ***@***.***> 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
<#68 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB5hbYHj2jXpPkKDETgmg4DCTsyQmpb5ks5sB919gaJpZM4NzSC9>
.
|
|
Well, errors in Would you mind updating the PR? |
|
Yep. Thanks for your attention and quick responses.
…On Thu, Jun 8, 2017 at 2:51 PM, Kirill Agalakov ***@***.***> 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
<#68 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB5hbcjdrbzVrF5dRWQzr3BZgXGNTQ_2ks5sB-BHgaJpZM4NzSC9>
.
|
|
Done. |
|
Great, thanks! Now we need @javivelasco to merge it |
when the source style is empty / contain only :global rules,
extract-text-webpack-pluginput string instead of json, which break the merge.this if prevent this specific case
#66