Expand syntax highlighting

[dreis2211]

Hi,

prior to this PR keywords like COPY or REFRESH MATERIALIZED VIEW were not highlighted due to using old versions of the sql set rather than using an up-to-date pgsql set of highlight.js.

Before

After

In order to achieve that I had to update quite some dev dependencies and highlight.js and also adjust the Gruntfile a bit due to highlight.js using ES6 these days, which broke the uglify step. The resulting JS file is 632KB while it had 594KB previously - I think that's acceptable, though.

Let me know what you think. Cheers, Christoph

[rjuju]

I'm not necessarily opposed to that, but it seems to have a lot of collateral changes.

At least, any change in the js build system should be in a separate commit, same for the bumps (if any) in the js dependencies.

As far as I can see the only bumped js lib are for the build system, not for the powa js app, right?

And just to be clear, I realized a few days ago that I wasn't able to build the js/css stuff anymore, so if you managed to fix that it's a very good news!

[dreis2211]

I only updated things in order to build the CSS and JS stuff properly.

Just let me know how I should proceed now.

[rjuju]

I only updated things in order to build the CSS and JS stuff properly.

Ok, that's what I thought, great! I also see that hightlight js isn't handled with the rest of the js libs in bower. I'm not sure why, but it's not your fault so no need to change it.

Just let me know how I should proceed now.

Could you split your commit in multiple commits:

• one to update the various things to fix the js build system,
• one to update the build libs (if that can be separated, otherwise in the same first commit)
• one to update highlight js and make the underlying compatibility change. Please also specify the highlightjs version in the commit message
• and finally one commit to switch to plpgsql set

The goal is to make the git history cleaner and easier to read for the next people who will look at it.

[dreis2211]

The updates of the build libs and the changes to the build system are closely related. I think I can only split this to one commit for those 2 points. Resulting in overall 3 commits.

[rjuju]

Yeah that's what I was thinking. 3 commits would be perfect then, thanks a ton!

[dreis2211]

Where do you want the resulting min CSS and min JS file? In the last commit or one additional commit for those?

[rjuju]

I think it's a matter of personal preference. I'm fine with both, so it's up to you!

[dreis2211]

@rjuju I just noticed that I can't really split the pgsql from the highlight js update. I chose do download only the pgsql language set (in order to keep the JS smaller). So the individual commit of only updating highlight js would leave things broken. Suggestions?

[rjuju]

Oh, indeed. Well, I guess that a smaller js file is better, so a single commit there will be perfect. It would be good to mention on the commit message (I don't think we have anywhere in the code that would be suitable for that?) the hljs version and that it only has (and should only have) the pgsql language set.

[dreis2211]

I chose to download the pgsql and the sql set again, it only increases the size slightly. By that I could make independent commits and we could theoretically jump back to only using the sql set if pgsql causes troubles for whatever reasons.

I think I have done everything, that you asked me to.

Let me know if this is sufficient for you, @rjuju

[rjuju]

I chose to download the pgsql and the sql set again, it only increases the size slightly. By that I could make independent commits and we could theoretically jump back to only using the sql set if pgsql causes troubles for whatever reasons.

It seems like a good idea to be able to switch back easily if needed, thanks for doing that!

I think I have done everything, that you asked me to.

I only did a few quick checks for now, but it seems perfect:

• I can now update the npm dependencies and run a grunt dist. As I mentioned in nearby issues I don't have much knowledge about frontend dev, so it's highly appreciated
• all the UI seems to work as usual
• the new highlight is really much better. One good think on top of the new keywords recognition is that the comments are also now recognized as such, which is another great improvement (look for instance at https://demo-powa.anayrat.info/server/0/database/powa/query/3134900691544350244/overview/ for non trivial query with comments wrongly highlighted)

I will do a more thorough review tomorrow just in case and merge the PR unless I find any problem.

[dreis2211]

Thanks for the feedback (and patience) so far, @rjuju .

Yeah, the highlighted comments are a nice side-benefit of the updates. Noticed that too already.

[rjuju]

@dreis2211 so looking deeper at the patchset I didn't see any problem. I only have 2 nitpicking comments:

• commit 524b93bd54efbe551466af3742740f6ca0694914 changed the indentation in Gruntfile.js from 2 spaces to 4 spaces. It's not really a problem as we don't really have an official position on that, but that's something we more or less tried to follow I think, given that js code tends to have a lot of nesting. I'm planning to add a .editorconfig soon to clarify that (mostly 4 space indent for python, 2 spaces for js/css...). I only bring that up because it makes the commit a bit hard to review as it adds some unnecessary diff chunks due to that change. This is definitely not a blocker as I would eventually change that (with the rest of the custom js files) when adding a .editorconfig anyway
• commit 5916806321f27d9bb246e201c4a125ecc4427fe3 updated higlightjs, but as far as I can see you used a non minified version of the library, and there is not build step to eventually minify it. Looking at the http requests, the app does serve a non minified version. I think that this should be fixed, I'm assuming by adding a minified version of the lib in our tree.

Is that ok for you?

[dreis2211]

I'm sorry for changing the indentation - it was inconsistent before (I think) and thus the file was hard to change.

It's the "minified" version - unfortunately it's not as minified as before. What I'm confused by is the fact, that the "non minified" version is served by the app. Isn't the debug flag locally enabled and thus you see tons of JS files being requested instead of just one powa.min-all.js? See layout.html. Having that said, I needed to turn off compression in the uglify step to fix building in general, but that should still minify things.

[rjuju]

I'm sorry for changing the indentation - it was inconsistent before (I think) and thus the file was hard to change.

No worries, it's indeed entirely inconsistent for now. I'm fine with the actual changes in the file so as I said it's not a blocker.

It's the "minified" version - unfortunately it's not as minified as before.

Oh ok. I'm used to see a single-line file that's why I raised it. If that's their new minified version, I'm fine with it!

Isn't the debug flag locally enabled and thus you see tons of JS files being requested instead of just one powa.min-all.js

Ah right I was using the debug version. I just rechecked with the release version and indeed I only get the massive powa-all.min.js.

I needed to turn off compression in the uglify step to fix building in general, but that should still minify things.

Yes I saw that. I'm not sure why it was a problem, but given the figures you showed in the first comment I have no problem with the few extra dozen kB (assuming it's coming from this change rather than something else).

So it seems all good to me. Do you have anything to add? If not I'll go ahead and merge this pull request!

[dreis2211]

I have nothing to add - thanks for your patience again.

[rjuju]

Thanks a lot for your work. You really brought great improvements!!