Skip to content

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

Merged
merged 10 commits into from
Aug 22, 2017
Merged

Conversation

IanVS
Copy link
Contributor

@IanVS IanVS commented Jul 19, 2017

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 a styleName 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 when silenceStyleNameErrors 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.

IanVS added 9 commits July 19, 2017 10:59
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.
@gajus
Copy link
Owner

gajus commented Jul 21, 2017

My slight problem with this is that silenceStyleNameErrors as a boolean operation is all or nothing, e.g. a developer might want to prevent errors from being thrown but want to print an error in the console log.

Therefore, something like handleNotFoundStyleName with options "throw", "log" and "ignore" would be a lot more versatile.

What do you think @IanVS ?

@gajus
Copy link
Owner

gajus commented Jul 24, 2017

@IanVS Are you comfortable with this change?

@IanVS
Copy link
Contributor Author

IanVS commented Jul 24, 2017

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 "throw", to match current functionality, so that's what I'll use unless I hear differently from you.

This changes the option from a boolean “silenceStyleNameErrors” to an
enum “handleMissingStyleName” option, with possible values of “throw”
(default), “warn”, and “ignore”.
@IanVS
Copy link
Contributor Author

IanVS commented Aug 20, 2017

Apologies that this took a while, but I've updated the PR to add a handleMissingStyleName option, with possible values of "throw" (default), "warn" (console.warn), and "ignore" (do nothing). Please let me know what you think.

@gajus
Copy link
Owner

gajus commented Aug 22, 2017

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.

@gajus gajus merged commit 3c4e6d0 into gajus:master Aug 22, 2017
@IanVS IanVS deleted the 97-ignore-errors branch August 22, 2017 18:34
@IanVS
Copy link
Contributor Author

IanVS commented Aug 25, 2017

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.

@gajus
Copy link
Owner

gajus commented Aug 25, 2017

No worries. Only natural when picking up new code base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignore Errors in Testing
2 participants