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

Refresh phase 2 #133

Merged
merged 30 commits into from
Mar 17, 2023
Merged

Refresh phase 2 #133

merged 30 commits into from
Mar 17, 2023

Conversation

jgonggrijp
Copy link
Contributor

Where #126 prepared the backend for #118, the current PR is doing the same for the frontend, including changes related to #125, #119, #117, #120, #121, #122, #130 and #131. I will push the actual separation, along with some final cleanup, in the third and last installment of this series.

I did the following things on this branch:

  • Fixed Annotations apparently cannot be deleted #125 by restoring a change that had been accidentally lost after a merge or rebase.
  • Made all frontend dependencies explicit in the package.json, upgrading most of them along the way, and started tracking the package-lock.json. The only remaining seriously outdated package is Bootstrap; updating that package can wait until later.
  • Moved all frontend Mustache templates into individual frontend modules, where they can be imported directly as functions by adjacent JavaScript modules. This was necessary both for Frontend unittesting harness with Mocha/Karma #117 and Separate frontend and backend #118, because in the old situation, the frontend templates were embedded in the index.html by the backend (they were sourced from vre/templates/vre/client_templates.html, which has now been deleted).
  • Added a frontend test suite based on Karma, Mocha, Sinon and power-assert.
  • Added unittests for a sizable portion of the frontend code; not all of it, but enough to serve as example code and to make it likely that we will catch breaking infrastructure changes before they pop up in production.
  • While writing the tests, I noticed and fixed a few minor code quality issues.
  • Changed the bundling tool from Browserify, which is CommonJS-native, to Rollup, which is ESM-native and has some additional nice perks.
  • Replaced Handlebars, which was used for rendering Mustache templates in compatibility mode rather than for Handlebars's own, slightly different templating language, by Wontache, a new dedicated Mustache engine by yours truly. This was originally on my wish list for a later stage, because Wontache is smaller and faster than Handlebars and the fact that I maintain it seems like an advantage, but I brought it forward because this made the move to Rollup easier.

Along the way, I also made some transitional changes that are no longer visible in the end result. I wanted to have the test suite running as soon as possible, so that I could verify that my infrastructure changes didn't break anything. This meant that I had to cater changes specifically to Browserify and Handlebars along the way, which later became obsolete.

By way of review, I suggest that @tijmenbaarda and @lukavdplas both do the following:

  • Form an opinion on the above list.
  • Review the changes in the package.json.
  • Review the rollup.config.js and the karma.conf.js for any surprises.
  • Review the changes in the README.md and check whether they seem clear, complete and consistent with the above.
  • Review a single frontend template (vre/static/vre/**/*.mustache) just to get a quick impression of what these templates look like.
  • Review a single frontend view (vre/static/vre/**/*.view.js) just to see the notation by which the template is imported.
  • Review a single frontend test module (vre/static/vre/**/*.test.js). Give it a hard stare and check for any surprises.

Copy link
Contributor

@lukavdplas lukavdplas left a comment

Choose a reason for hiding this comment

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

This looks good to me!

Copy link
Contributor

@tijmenbaarda tijmenbaarda left a comment

Choose a reason for hiding this comment

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

Sorry for the late review! I have a few minor comments.

jgonggrijp added a commit that referenced this pull request Mar 13, 2023
jgonggrijp added a commit that referenced this pull request Mar 13, 2023
@jgonggrijp
Copy link
Contributor Author

Thanks to both for your reviews! I have processed Tijmen's comments. This branch should be rebased on develop after #127 has been merged and then it can be merged as well.

These changes seem to have falled out either in 6e026c7 or 47a5012.
This is needed because of a subtle breaking change in Bootstrap 3.4.
The main purpose of these changes was to replace Browserify by Rollup
(close #121), but it turned this was easier to do if also replacing
Handlebars by Wontache (close #120).
Updates Babel along the way (#119).

These changes cut the bundle size nearly in half, even without
minification.
We are no longer using Handlebars, so the workaround is no longer
needed (#120).

This reverts commit a446996.
@jgonggrijp jgonggrijp merged commit 53c0efe into develop Mar 17, 2023
@jgonggrijp jgonggrijp deleted the feature/refresh-2 branch March 17, 2023 12:27
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.

Annotations apparently cannot be deleted
3 participants