Skip to content

Fix/vulnerabilities cheerio - #115

Merged
jonkemp merged 8 commits into
jonkemp:masterfrom
pleymor:fix/vulnerabilities-cheerio
Jul 6, 2022
Merged

Fix/vulnerabilities cheerio#115
jonkemp merged 8 commits into
jonkemp:masterfrom
pleymor:fix/vulnerabilities-cheerio

Conversation

@pleymor

@pleymor pleymor commented May 27, 2022

Copy link
Copy Markdown
Contributor

fixes vulnerability on cheerio

@jonkemp

jonkemp commented Jun 2, 2022

Copy link
Copy Markdown
Owner

No need to bump the version numbers. That will happen on publish.

@pleymor
pleymor force-pushed the fix/vulnerabilities-cheerio branch from 623e05a to 0358e5d Compare June 3, 2022 15:35
@pleymor

pleymor commented Jun 3, 2022

Copy link
Copy Markdown
Contributor Author

✔️ done: the versions are no longer modified.
@jonkemp can you double check if the tests I modified are still ok for you?
Thanks! 🙏

@jonkemp

jonkemp commented Jun 4, 2022

Copy link
Copy Markdown
Owner

Why are there .nvmrc files?

@pleymor

pleymor commented Jun 4, 2022 via email

Copy link
Copy Markdown
Contributor Author

@jonkemp

jonkemp commented Jun 14, 2022

Copy link
Copy Markdown
Owner

Sorry for the delayed response on this. I don't want to include all the .nvmrc files so if you can think of another solution, that would be preferred.

@pleymor

pleymor commented Jun 14, 2022

Copy link
Copy Markdown
Contributor Author

Just deleted the files :)

@jonkemp

jonkemp commented Jun 15, 2022

Copy link
Copy Markdown
Owner

The .nvmrc file in the root is still there.

Also, why did the test output change?

@jonkemp

jonkemp commented Jun 17, 2022

Copy link
Copy Markdown
Owner

Please use js-beautify for the unit test comparisons. Thanks.

@pleymor

pleymor commented Jun 17, 2022

Copy link
Copy Markdown
Contributor Author

Hello @jonkemp, I'm done removing the last .nvmrc (sorry for the omission).

in unit tests, html is compared, not js. Do you want I try html-beautify to replace this home made function?

const expected = String(fs.readFileSync(expectedPath)).replace(/(\r\n|\n|\r)/gm, "");
html.replace(/(\r\n|\n|\r)/gm, "").should.be.equal(expected);

@jonkemp

jonkemp commented Jun 17, 2022

Copy link
Copy Markdown
Owner

const beautify = require('js-beautify').html; will accomplish the same thing.

@pleymor

pleymor commented Jun 23, 2022

Copy link
Copy Markdown
Contributor Author

@jonkemp beautify is now used in tests ✔️

Comment thread lib/setTableAttrs.js Outdated
@@ -1,4 +1,4 @@
const cheerio = require('cheerio');
const cheerio = require('cheerio').default;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is causing a lint error.

`eslint --fix index.js lib/ packages/`
@pleymor

pleymor commented Jun 24, 2022

Copy link
Copy Markdown
Contributor Author

Lint error fixed. Note that there are 3 lint warnings left due to complexity, that could be fixed quite easily in a dedicated PR

@coveralls

coveralls commented Jun 24, 2022

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.02%) to 95.918% when pulling c44f378 on pleymor:fix/vulnerabilities-cheerio into 21fb57e on jonkemp:master.

@GerardSetho

Copy link
Copy Markdown

Thanks @pleymor for the effort! @jonkemp Is this ok to merge?

@jonkemp
jonkemp merged commit b4dba50 into jonkemp:master Jul 6, 2022
@GerardSetho

Copy link
Copy Markdown

Thank you @jonkemp !
Any updates on when this will be published to NPM?

@jonkemp

jonkemp commented Jul 7, 2022

Copy link
Copy Markdown
Owner

Tests are failing locally so as soon as I can get those passing.

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.

5 participants