Skip to content
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

[New]: jsx-no-literals: add ignoreProps option to ignore props validation #2146

Merged
merged 1 commit into from Mar 11, 2020

Conversation

@iiison
Copy link
Contributor

@iiison iiison commented Jan 29, 2019

  • Update Tests Accordingly
  • Update Rule Readme file
  • Rule shouldn't allow strings in empty tags

Fixes #2142

…lidation

Co-authored-by: Bharat Soni <i.bharat.soni@gmail.com>
Co-authored-by: Serg Hospodarets <shospodarets@gmail.com>
@iiison
Copy link
Contributor Author

@iiison iiison commented Feb 7, 2019

@ljharb added new option to validate props. I've set it to true by default so that behaviour stays the same, user can set the value to false in order to remove props validation.

One more thing, rule passes if prop is a strings literal(<div className="abc" />), but if the prop value is a template literal, then rule will throw error. This behaviour was there from starting, LMK if you want to change this.

@iiison
Copy link
Contributor Author

@iiison iiison commented Feb 7, 2019

@ljharb the build is breaking from starting, it works fine in my local, is it because I am using ES6 syntax? Could you please help me if you are aware of the error..


* `noStrings` - Enforces no string literals used as children, wrapped or unwrapped.
* `validateProps` - Enforces no template literals used as props.

This comment has been minimized.

@ljharb

ljharb Feb 7, 2019
Collaborator

why only template? i'd expect any literals to be detected by this option

This comment has been minimized.

@iiison

iiison Feb 7, 2019
Author Contributor

I was trying to mimic the same behaviour as before, will detect string literals too.

docs/rules/jsx-no-literals.md Outdated Show resolved Hide resolved
tests/lib/rules/jsx-no-literals.js Show resolved Hide resolved
@iiison
Copy link
Contributor Author

@iiison iiison commented Feb 7, 2019

@ljharb I see you removed your comment to detect and disallow string literals in props, shall I add that feature or not..

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 7, 2019

@iiison i marked it resolved because you indicated you'd incorporate it. There's no reason for a rule called "literals" to differentiate between string and template.

@iiison
Copy link
Contributor Author

@iiison iiison commented Mar 27, 2019

@ljharb my laptop was broken for some time, can you please provide the context of the last comment that you resolved.. I will fix it as soon as I get a little context of it.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Mar 27, 2019

@iiison see #2146 (comment), i've unmarked it as resolved. basically, we want template and string literals checked.

@iiison
Copy link
Contributor Author

@iiison iiison commented Apr 3, 2019

@ljharb if we don't allow strings in props, then you need to declare variables for all props like type attribute in input tag element and for a lot of other places too. I think this will complicate things. Let me know your thoughts.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Apr 6, 2019

@iiison sure, that's why you might not want to enable the option. There's no difference between foo="bar" and foo={"bar"} and foo={`bar`}.

@iiison
Copy link
Contributor Author

@iiison iiison commented Apr 22, 2019

@ljharb updated the PR. Added requested changes. But build is failing. The code is failing in few versions of Node(not sure why), can you please help me with that.

const sourceCode = context.getSourceCode();
create: function({
report,
options: [configs = {}],

This comment has been minimized.

@ljharb

ljharb Apr 22, 2019
Collaborator

does this syntax work in node 4?

create: function(context) {
const isNoStrings = context.options[0] ? context.options[0].noStrings : false;
const sourceCode = context.getSourceCode();
create: function({

This comment has been minimized.

@ljharb

ljharb Apr 22, 2019
Collaborator

let's try going back to the non-destructuring form.

} = configs;
const isNoStrings = noStrings;
const shouldValidateProps = validateProps;
const sourceCode = getSourceCode();

This comment has been minimized.

@ljharb

ljharb Apr 22, 2019
Collaborator

in particular, it seems that eslint 3 relies on the receiver (the this value) of getSourceCode being context.

@iiison
Copy link
Contributor Author

@iiison iiison commented Apr 25, 2019

fixed all errors!

@iiison
Copy link
Contributor Author

@iiison iiison commented Apr 26, 2019

@ljharb Let me know if you want to add more changes to the PR.

{message: stringsMessage('\'bar\'')}
]
}, {
code: '<Foo event={() => {}} />',

This comment has been minimized.

@ljharb

ljharb Apr 26, 2019
Collaborator

this rule is about string literals. why would a function value ever trigger an error?

This comment has been minimized.

@iiison

iiison Apr 26, 2019
Author Contributor

I suppose I went a little too far on this. Will remove the logic.

```

```jsx
var Hello = <div event={()=>{}} />;

This comment has been minimized.

@ljharb

ljharb Apr 28, 2019
Collaborator

should this be removed from the readme?

@iiison
Copy link
Contributor Author

@iiison iiison commented Apr 29, 2019

@ljharb I'm not sure why, but AppVeyor is failing out of nowhere. I just deleted 2 lines from the readme file, not sure why it is breaking.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Apr 29, 2019

I'm not sure either, looks like a jest error - either way, could you try rebasing on latest master instead of merging?

@iiison
Copy link
Contributor Author

@iiison iiison commented Apr 29, 2019

The build was failing when I didn't merge the master in the branch. but will try the rebase as well.

@iiison iiison force-pushed the iiison:string-litral-fix branch 2 times, most recently from 5ac178c to 29d70e6 Apr 29, 2019
@iiison
Copy link
Contributor Author

@iiison iiison commented Apr 29, 2019

rebased with the master, still, the build is failing with the same error.

@iiison
Copy link
Contributor Author

@iiison iiison commented Apr 29, 2019

@ljharb build is also failing for the master

@ljharb ljharb force-pushed the iiison:string-litral-fix branch from e76c282 to 8a1eb1e Apr 30, 2019
@ljharb
ljharb approved these changes Apr 30, 2019
@ljharb
Copy link
Collaborator

@ljharb ljharb commented Apr 30, 2019

Blocked by #2253.

@iiison
Copy link
Contributor Author

@iiison iiison commented May 3, 2019

@ljharb as #2253 is closed, can we merge this branch...

@mauricecruz
Copy link

@mauricecruz mauricecruz commented Oct 22, 2019

@iiison any interest in moving this forward? Happy to take a look if you're no longer vested in this fix.

@iiison
Copy link
Contributor Author

@iiison iiison commented Oct 30, 2019

@mauricecruz Please go ahead. I won't be able to work on this for a while.

@dbassett17
Copy link

@dbassett17 dbassett17 commented Nov 24, 2019

@mauricecruz @ljharb @iiison Any progress on this or is it still blocked? If nobody else has the time I can probably find some time to finish this up soon.

@ljharb ljharb changed the title FIX: Don't validate props in jsx-no-literals rule [New]: jsx-no-literals: add validateProps option to ignore props validation Nov 28, 2019
@ljharb ljharb force-pushed the iiison:string-litral-fix branch 2 times, most recently from 448f86c to cf2210c Nov 28, 2019
errors: [{message: stringsMessage('`Test`')}]
}, {
code: '<Foo bar={`${baz}`} />',
options: [{noStrings: true}],
options: [{noStrings: true, validateProps: true}],

This comment has been minimized.

@ljharb

ljharb Nov 28, 2019
Collaborator

These test changes show that the option (that doesn't exist yet) seems to already be true by default prior to this PR. In that case, this option should probably be more like ignoreProps: true which defaults to false.

This comment has been minimized.

@SalimBensiali

SalimBensiali Feb 25, 2020

@ljharb just checking... Is this the only pending requested change?

This comment has been minimized.

@ljharb

ljharb Mar 6, 2020
Collaborator

@Gasparila at the moment, yes - i'll have to give it a rereview after it's next updated

This comment has been minimized.

@malyw

malyw Mar 8, 2020
Contributor

Just found this great react eslint rule to use in a big product and faced the same problem that it goes through the props, which was a bit surprising, but ok.
In short, the current rule behavior is to check props, facilitating it to be disabled by default may break some existing users functionalities, who may rely on this.
+1 , is should be either changed to ````validateProps(true` default)``` in the current naming, or switched to `ignoreProps`

@ljharb ljharb force-pushed the iiison:string-litral-fix branch from cf2210c to 6dd14b9 Nov 28, 2019
@ljharb
Copy link
Collaborator

@ljharb ljharb commented Dec 13, 2019

ping @iiison, are you still interested in completing this PR?

@SalimBensiali
Copy link

@SalimBensiali SalimBensiali commented Feb 24, 2020

@ljharb @iiison, any update on this? Are there any requested changes still pending? Happy to chip in.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 25, 2020

@SalimBensiali if you want to post a link to your branch with the needed changes (do NOT post a new PR, please) then i'm happy to pull it in.

@malyw
Copy link
Contributor

@malyw malyw commented Mar 8, 2020

Hey @ljharb @SalimBensiali @iiison
I created a PR to switch to ignoreProps: true and adjusted the code, docs and tests to pass: iiison#1
It targets the current branch, can you please review when you have time, and let me know if you'd need me to merge it, or you'd prefer other way of having this arrived in the main repo

@ljharb ljharb force-pushed the iiison:string-litral-fix branch 2 times, most recently from 122721f to 13e649e Mar 10, 2020
@ljharb ljharb changed the title [New]: jsx-no-literals: add validateProps option to ignore props validation [New]: jsx-no-literals: add ignoreProps option to ignore props validation Mar 11, 2020
@ljharb ljharb force-pushed the iiison:string-litral-fix branch from 13e649e to fa863ce Mar 11, 2020
@ljharb
ljharb approved these changes Mar 11, 2020
Copy link
Collaborator

@ljharb ljharb left a comment

Thanks @iiison and @malyw!

@ljharb ljharb merged commit fa863ce into yannickcr:master Mar 11, 2020
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.009%) to 97.616%
Details
@mauricecruz

This comment has been minimized.

Copy link

@mauricecruz mauricecruz commented on fa863ce Mar 23, 2020

Is there a release schedule or plan to add this to a release? Couldn't find anything on release cadence in the readme.

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb replied Mar 23, 2020

Everything in master is planned to go in the next release; there's no committed cadence.

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

Successfully merging this pull request may close these issues.

6 participants