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

Rewrite javascript #221

Merged
merged 12 commits into from Mar 17, 2020
Merged

Rewrite javascript #221

merged 12 commits into from Mar 17, 2020

Conversation

@Reamer
Copy link
Member Author

Reamer commented Feb 26, 2020

@Vampire
I uploaded a new pre-release version. The JavaScript functions should now work. Can you confirm?
The changed CSS-style is an acceptable regression.

@Vampire
Copy link

Vampire commented Feb 26, 2020

Only partly.
If you for example try to expand the Evidence block, it does not work.

And imho it is not an acceptable regression.
It is very much harder to read without the proper formatting.
And it also affects the functionality.
That the Evidence block expanding does not work is also related to the missing CSS classes from the original report.

And besides that it looks ugly and does not work properly, it will also cause false-positives reported about dependency check itself, as people will report that the report looks ugly or something does not work, because they only look at it from SQ.
Even if you now work-around all issues we find, each new version of dependency check could change the report slightly and by that break the integration you are knitting.
Imho showing the report as is in an iframe was exactly the right thing to do.

@Reamer
Copy link
Member Author

Reamer commented Mar 3, 2020

Hi @Vampire,
thanks for your opinion. You are right, an iframe is the only option to separate SonarQube from potential manipulated HTML-Report.

@Reamer
Copy link
Member Author

Reamer commented Mar 4, 2020

Now working it's working with iframe. CSS and JavaScript works without problems in an iframe.
The default size of an iframe is 150px, which is really small. Tried a little bit to make the size dynamic, but doesn't found any solution. I think a static value of 720px is a good first start.

@thib3113
Copy link
Contributor

thib3113 commented Mar 5, 2020

Hello,
why did you eject react-scripts ?
In my opinion, it's generally not a good idea, because playing manually with dependencies is a nightmare ....

Another things, doing a request in a componentDidMount is not a good idea ... except if you have a component that never dismount (seems to be you'r case) .

And because I see it in an exception, instead of [].filter(...)[0] you can use [].find(...) .

Else, if you need help with js, I can help . (just I'm not an expert to build / install java projects)

@Reamer
Copy link
Member Author

Reamer commented Mar 7, 2020

Hi @thib3113,
I'm no JavaScript developer and this commits are my first steps with react. If you can improve the code, it would be awesome.
I used the official project sonar-custom-plugin-example as a base - in particular the VersionsMeasuresHistoryApp.js class.

why did you eject react-scripts ?

I have no idea what you mean?

Another things, doing a request in a componentDidMount is not a good idea ... except if you have a component that never dismount (seems to be you'r case) .

That logic is based on VersionsMeasuresHistoryApp.js line 33 and api.js line 47.
I think the dismount of the component is done, when you change to another SonarQube page. If you switch back to dependency-check report page, you may get the newest/actual report.
I know the additional request takes time. We have a spinner to bridge the time.

And because I see it in an exception, instead of [].filter(...)[0] you can use [].find(...) .

I'll check this.

Else, if you need help with js, I can help . (just I'm not an expert to build / install java projects)

I need help with JavaScript. Take a additional look into #217.

In general I liked the development with the npm proxy server. Take a look into sonar-custom-plugin-example how to use it. The build of the final JavaScript file with yarn in maven is a good and optimized.

@thib3113
Copy link
Contributor

thib3113 commented Mar 7, 2020

why did you eject react-scripts ?

I have no idea what you mean?
Arf, yes, the base project already eject it ....

In fact, react-scripts, is a dependency, that come with all dependencies to build a react project ( so babel version X.X.X, with plugins version X.X.X ), ejecting react-scripts, allow you to customize the build process, but you need to update all the version manually (and some time, last version of babel, doesn't work with last version of plugins ... and you need to try lot of times to find the good versions ... react-scripts solve that) (on a projet, I eject react-scripts to use less instead of sass ... Finally, converting all the project to sass is really better than managing dependencies manually)

About this, you can install some bots on the github repo (like greenkeeper, or dependabot something like that), that will run a CI to update npm dependencies (and adding a CI test, that just try to build the project to start)

That logic is based on VersionsMeasuresHistoryApp.js line 33 and api.js line 47.
I think the dismount of the component is done, when you change to another SonarQube page. If you switch back to dependency-check report page, you may get the newest/actual report.
I know the additional request takes time. We have a spinner to bridge the time.

Ok ... So, if you are loading data, and the component is dismount before the end of the loading, you will call setState() on an unmounted component . And react will show a warning . ( it's just a warning, and I think thats why it's done like that, to win time ) .

So, it's a bad pratice, but it is really important for this little part ?

I need help with JavaScript. Take a additional look into #217.

Yes, I saw it, and that's because I've add a comment here .

What did you need to do ?

In my opinion, doing a custom page, generated from data coming from dependency check (so reading the dependency-check.json), is better than showing an iframe (but more work) .

First, in general, iframe is not a good option (security, headers, proxy ... ),
second, html report from dependency check is sooooo huge some times . Maybe, if you can "store" the html (not in a DB, but on the file system), a better option can be to add a "download link", that read the html, and the plugin page show some things from the json data .
(or better, rework the html builded by dependency check, but not the same project)

@thib3113
Copy link
Contributor

thib3113 commented Mar 8, 2020

So I've done some search :

  • about the react scripts eject, the build process seems to doesn't fit with the react scripts build process . So, we can't use react scripts.

  • about the setState, getJson doesn't return a way to cancel the request ( react say to cancel the request on unmount in an example : https://fr.reactjs.org/blog/2018/03/27/update-on-async-rendering.html#fetching-external-data ), and I think using a store system like redux is too big for only one component . So, to "hide the warning", we can set a var mounted to true in componentWillMount, and to false in componentWillUnmount, and call setState only if mounted === true, but it's just a way to hide the warning

@Reamer
Copy link
Member Author

Reamer commented Mar 9, 2020

Thanks for doing some search.
You wrote about some react warning. How can I trigger this "warning"?

First, in general, iframe is not a good option (security, headers, proxy ... )

I doesn't like iframes either. This plugin uploads a complete HTML-Page via a very hacky way with all compnents (pictures, css and javascript) to Sonarqube. This HTML-Page isn't generated by this plugin, nor I can't exclude that another processes manipulate the HTML page after dependency-check and this SonarQube plugin. An iframe is a very secure way to separate the HTML page and sonarqube.

At the moment iframes are not blocked by SonarQube. because of that, I'll follow the way to serve the HTML within an iframe.

Maybe, if you can "store" the html (not in a DB, but on the file system), a better option can be to add a "download link", that read the html, and the plugin page show some things from the json data .

I know of no way to store a file on SonarQubes filesystem. I think a filesystem is much more complicated than a database in a cluster setup for sharing information exchange.

In my opinion, doing a custom page, generated from data coming from dependency check (so reading the dependency-check.json), is better than showing an iframe (but more work) .

We can build a second custom page, generated from data coming from dependency check. At the moment you have the generated issues and some metrics.
Uploading dependency-check.json should be avoided. Long time ago this plugin was delivered with an widget. Widget support was drop by SonarQube, therefore I deleted related files. Maybe we can reactivate this widget information
If you need help to include a second page - I'm here to help.

What did you need to do ?

At the moment. I have one big issue. The iframe has a static size 720px. Maybe you know a way, to make the size dynamic.

@thib3113
Copy link
Contributor

thib3113 commented Mar 9, 2020

You wrote about some react warning. How can I trigger this "warning"?

hum, you need to "dismount" the component before the end of the getJson XHR request, so, maybe in chrome devtools, set a really slow connection, and dismount the component (maybe by changing tab ? or changing project ? just something, to hide the component, without reloading the page, but maybe the component is never dismount, because sonarqube interface doesn't use react)

I know of no way to store a file on SonarQubes filesystem. I think a filesystem is much more complicated than a database in a cluster setup for sharing information exchange.

Yes, I understand ... A way to do this can be to use object storage or something like that, but huge modifs, and sonarqube maybe doesn't integrate it, if they doesn't need it . Else, if I remember, you use SQL to store this, can you create a custom table, with html stored as blob ? And delete the old html ? (can we access previous html report ?) (but this pull request is maybe not the place to discuss about it) .

We can build a second custom page, generated from data coming from dependency check. At the moment you have the generated issues and some metrics.
Uploading dependency-check.json should be avoided. Long time ago this plugin was delivered with an widget. Widget support was drop by SonarQube, therefore I deleted related files. Maybe we can reactivate this widget information
If you need help to include a second page - I'm here to help.

I doesn't really know the widgets for sonarqube (I'm not using sonarqube for a long time).
About the sceond page, I doesn't know how to get data, or which design we can do (what data to show ? ).

My idea, is just to do something more "simple" than the dependency check html page, for node.js for example, I'm not sure showing the list of files is really helpful (instead of showing the dependencies) (I think java show only .jar files, so with java, it's not really a problem to show each files) .

At the moment. I have one big issue. The iframe has a static size 720px. Maybe you know a way, to make the size dynamic.

I never use the flex ... So maybe with css, but doesn't know .
Else, with react, getting the height of the iframe content ? and set it to the iframe ?
I can access the height of the iframe by doing $0.contentDocument.defaultView.outerHeight (so $0 is the this for the iframe, donc maybe on React iframe provide a "onLoad" props ?
something like that :
return (<iframe src="" onLoad={function(e){console.log(this.contentDocument.defaultView.outerHeight)}} />);
Show something in the console ? ( with chrome debugger, I get "1400" )

At the moment iframes are not blocked by SonarQube. because of that, I'll follow the way to serve the HTML within an iframe.

Yes, But I know some vulnerabilities are present with iframe, or proxy can't block some iframe, or things like that ... ( and on a story, firefox block the iframe too, and you need to use the same scheme to call the iframe, else mixed content can produce error too .. ) . ( but it's an hacky way, so need to do the best )

@sonarcloud
Copy link

sonarcloud bot commented Mar 17, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@Reamer Reamer merged commit 00fe53e into master Mar 17, 2020
@Reamer
Copy link
Member Author

Reamer commented Mar 17, 2020

Hi @thib3113,
thanks for your input. I wrote 36187f9 for dynamic height.
I know it's not perfect, but it should sufficient.

Even with a slow connection (2G) I was not able to see the warning. As long as the data isn't loaded, the spinner is displayed.

create a custom table, with html stored as blob

Plugins doesn't have direct access to SonarQube-DB. It's only possible to add content via API calls.

Also thanks to @Vampire for your input.

@Reamer Reamer mentioned this pull request Mar 17, 2020
@Vampire
Copy link

Vampire commented Mar 24, 2020

Just tried the latest master, works great there, thanks

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

Successfully merging this pull request may close these issues.

None yet

3 participants