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

[BUG] Dependency library - Highlight.js for helper-markdown throwing error #284

Closed
ManuGM91 opened this issue Nov 19, 2020 · 21 comments · Fixed by #285
Closed

[BUG] Dependency library - Highlight.js for helper-markdown throwing error #284

ManuGM91 opened this issue Nov 19, 2020 · 21 comments · Fixed by #285
Labels
type: bug Something isn't working

Comments

@ManuGM91
Copy link

ManuGM91 commented Nov 19, 2020

Highlight.js for helper-markdown has reached EOL and is no longer supported. Please read this highlightjs/highlight.js#2877. The depreciation is instrumented by a post install script rather than npm deprecate and this is causing error when we use newman run command and include htmlextra as reporter.

Since this is a dependency library, I am having issue to upgrade it to version 10.

Even the latest htmlextra version is using highlight.js version 9. I have attached the screenshot for error message from Powershell command execution.

Always happy to share any more relevant information if required.

image

@ManuGM91 ManuGM91 added the type: bug Something isn't working label Nov 19, 2020
@joshgoebel
Copy link

Hmmmm... what exactly is returning an error? When I run npm install (and the notice is shown) the status returned is still 0... there should only be a visual issue here, not a hard error. I'm missing something.

@joshgoebel
Copy link

I see now. It looks like it's the warning that's the problem here, not the post-install script... I hadn't considered people treating the warning as a hard error (that is possibly an overreach), we use warnings in lots of places to communicate important warnings (but not full on errors) - and I haven't seen this usages cause any problems before now.

@ManuGM91
Copy link
Author

To be clear it's not the npm install which is throwing the error. npm install is just showing me the banner. The step which throws error for me is the newman run command, where I pass the newman htmlextra reporter as one of the argument.

@ManuGM91
Copy link
Author

To add to that, the npm WARN's like below isn't being considered as error.

npm WARN deprecated resolve-url@0.2.1: https://github.com/lydell/resolve-url#deprecated
npm WARN newman-reporter-htmlextra@1.14.0 requires a peer of newman@>=4 but none is installed. You must install peer dependencies yourself.

@joshgoebel
Copy link

To add to that, the npm WARN's like below isn't being considered as error.

So confusing. Not sure what npm is doing but Node.js considers warn equivalent to error, which is news to me - and wrong IMHO.

https://nodejs.org/api/console.html#console_console_warn_data_args

The console.warn() function is an alias for console.error().

But now we'll likely need to fix this on our end, though the deprecation of v9 of course is still relevant.

@DannyDainton
Copy link
Owner

Hey @joshgoebel - it feels like you know a lot more about this and highlight.js than I ever will.

Any hints or directions to point me it to update certain areas?

@joshgoebel
Copy link

@DannyDainton I'm the Highlight.js maintainer. Is Highlight.js in your dependency tree anywhere? If not and you're only throwing an error because of stderr then this issue was misfiled and really the issue is with us - that we shouldn't be using console.warn - which I'm looking into rectifying.

@DannyDainton
Copy link
Owner

DannyDainton commented Nov 19, 2020

I'm using it via a CDN on the report templates which I don't think would impact anything here.

I also have it here for helper-markdown

"highlight.js": "^9.12.0",

and here

"highlight.js": {

@joshgoebel
Copy link

It's going to keep logging deprecation notices (though no longer warnings/errors) until they upgrade to v10 or you hack the environment (see release notes when we push 9.18.5). You should consider encouraging them to do so. If you/they are running any user provided input thru the highlighter it's quite likely in the future you'll expose yourself to ReDOS attacks or similar.

You should upgrade your CDN usage if it's not v10 already. :-)

@DannyDainton
Copy link
Owner

I've made the local changes for the CDN versions so I'll push that out in the morning 😁

@ManuGM91
Copy link
Author

ManuGM91 commented Nov 20, 2020

Hi @DannyDainton , This issue is now resolved for me. @joshgoebel has released 9.18.5 of highlight.js with the fix which removed the hard error. Then I did an uninstall and install of reporter again, which automatically picked up the 9.18.5 release. Now the command newman run is not throwing the hard error as it was earlier.

So I am happy to close this issue now. Please let me know your thoughts.

For anyone who are facing the issue, I am sharing the original discussion link for the issue here again - highlightjs/highlight.js#2877.

@ManuGM91
Copy link
Author

ManuGM91 commented Nov 20, 2020

I've made the local changes for the CDN versions so I'll push that out in the morning 😁

So will this be a new version/release? Also, if I am using a custom html template, shall I update the below place alone in template?

<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/9.13.1/styles/github-gist.min.css">

@joshgoebel
Copy link

<script src="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/9.13.1/highlight.min.js"></script>
also seems relevant :)

@DannyDainton
Copy link
Owner

I was only planning on pushing a fix for the templates to swap out the CDN links.

As you're using a custom template, that's obviously not going to change that 😁

@joshgoebel
Copy link

@ManuGM91 If you're just using highlighting on the client side in your template (via CDN) then you should check the v10 README. But really you'd just need to upgrade the CDN links for BOTH highlight.js js and css files.

@DannyDainton DannyDainton linked a pull request Nov 20, 2020 that will close this issue
@DannyDainton
Copy link
Owner

@joshgoebel

Updated all the things that I can but still seeing thins in the CLI - still down to help-markdown to update :(

image

It's not stopping the reporter from installing but it looks bloody awful :D

@joshgoebel
Copy link

Check the release notes you can set an environment variable to hide it if it really bothers you, but it shouldn't cause any harm just being displayed.

@DannyDainton
Copy link
Owner

Depends how you define harm - I had someone on a Postman live stream last night telling people that my reporter was depreciated and was looking for a new one...just because they saw this when installing :D

I'll check out the release notes. Thanks! 🏆

@joshgoebel
Copy link

Hmmm, well that is annoying, but I'd also say they clearly weren't reading. :-) If someone was willing to contribute a PR I think we might be ok with a different notice for transitive dependencies vs direct. IE, check package.json and if highlight.js is not a direct dependency (dev or live) then print a different message.

The package newman-reporter (or one of it's dependencies) depends on Highlight.js version 9.

Version 9 is no longer supported... ... please file an issue with `newman-reporter` or...

Of course it might be possible to track down the exact dependency also, not sure what APIs might be ready available vs npm, etc... might also be able to do that just from package.json.lock though no everyone has one of those.

I'd write the wording if someone else wanted to actually contribute the dependency check logic.

@joshgoebel
Copy link

Anyone wanting to try that would want to do so on the 9-18-stable branch.

@github-actions
Copy link

github-actions bot commented May 4, 2022

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants