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

Confusing variable assignment - Eslint should be configured for this project #251

Open
ctaschereau opened this issue Mar 2, 2022 · 2 comments

Comments

@ctaschereau
Copy link

I was tracking down an issue I had and was reading the code when I saw this :

js-xss/lib/xss.js

Lines 133 to 140 in daa471e

// if enable stripIgnoreTagBody
var stripIgnoreTagBody = false;
if (options.stripIgnoreTagBody) {
var stripIgnoreTagBody = DEFAULT.StripTagBody(
options.stripIgnoreTagBody,
onIgnoreTag
);

Even my IDE was confused by this and we both thought (upon first glance) that the line 137 was shadowing the one on 135 but that was not the case since this project is using "var" instead of "let".

Since the var keyword hoists the declaration to the top of the function, the line 137 does not in fact shadow the previous one, but that is very confusing when reading the code.

I think that the easiest fix would be to simply remove that var keyword on line 137. A better fix would also include converting this project to use the let keyword. Eventually, setuping eslint correctly would have caught something like this.

@leizongmin
Copy link
Owner

Hi @ctaschereau , I agree with you. I think this project may need to be rewritten using ES6 syntax in the future. I'll do it if I have time.

@lumburr
Copy link
Contributor

lumburr commented Mar 9, 2022

I am very willing to contribute to the js-xss. I think it's possible to add an eslint first to improve code readability. I'm also more than willing to help if you want to rewrite in es6/typescript.

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

No branches or pull requests

3 participants