-
-
Notifications
You must be signed in to change notification settings - Fork 161
Add silenceStyleNameErrors
option
#112
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
Conversation
The double quotes that the plugin adds go against the linting config of the project, and editors that auto-fix linting rules will try to convert quotes in `expected.js` files to single quotes, causing tests to fail. It seems safest to ignore all fixture files from linting.
The only option is to silence styleName errors, for example in production. If `silenceStyleNameErrors` is true, the styleNames which cannot be found will simply be removed from the resulting classNames, matching the behavior of `react-css-modules`. This commit alone will not result in a change in behavior, the next steps will be to pass in the option from the babel-plugin.
This hooks up the options set by the user into getClassName, so that it has an effect. However, getClassName can also be used at runtime, which this commit does not address.
This commit adds the options object for getClassName iif silenceStyleNameErrors is true. By not always including the options object, it helps to save a few characters in the generated code.
Seeing the flow types first thing in the Configuration section was confusing to new users, so this adds a brief example of how to set babel plugin options, and moves the flow types underneath the table of options.
Flow was already not happy with using `InputObjectType` within its own definition, and was converting it to `{[key: string]: string | any}` when I inspected it in Nuclide. I’m honestly not sure how to specify a union type on an indexer as was being attempted, because flow complains as soon as you try to provide an object that has a property with a typed value.
My slight problem with this is that Therefore, something like What do you think @IanVS ? |
@IanVS Are you comfortable with this change? |
Yes, I think so. Normally I'm not a fan of string options due to the possibility of typos, but since you've set up option validation that's not really a concern. I'll work on making the changes within the next few days. Question: what would you like to be the default setting? I assume it should be |
This changes the option from a boolean “silenceStyleNameErrors” to an enum “handleMissingStyleName” option, with possible values of “throw” (default), “warn”, and “ignore”.
Apologies that this took a while, but I've updated the PR to add a |
Just came to complain why this hasn't been done yet, but it looks like I am show shopper. I am joking of course. Sorry, I've overlooked the notification. Looks perfect. |
Looks like I made some bugs with this one, sorry about that. I admit I had a bit of a hard time understanding how the various pieces all interact. |
No worries. Only natural when picking up new code base. |
Fixes #97
The main work in this PR is to add a new option that will let users silence the
Could not resolve the styleName...
errors when astyleName
is not found in a loaded CSS module. These kinds of errors can be super helpful in development, but during production users may want to prevent crashing their javascript and opt for broken styles instead.I decided not to use the same option name as react-css-modules errorWhenNotFound, because I find that options which default to
true
are more difficult to implement and reason about.I've tried to do this as cleanly as possible, but it did involve passing down an options object several levels to
getClassName
, as well as providing that options object in runtime whensilenceStyleNameErrors
is enabled. If you have suggestions for a better way to do this, please let me know and I'll adjust.Other minor changes
I've added an
.eslintignore
file that ignores the text fixture files. My editor kept trying to auto-correct them, and while I could have just turned that feature off, I think it's safer to ignore those files since they don't follow the linting rules anyway.I added a small example to the top of the configuration section in the readme, and moved the flow types after the options table. When I was first setting up this plugin I found it confusing to have those types at the top with no explanation, so hopefully this change is helpful.