search

intermine / InterMine-Data-Browser-Tool

InterMine Data Browser: a tool for exploring semi-homogeneous biological datasets
http://data-browser.apps.intermine.org/
Other
7 stars 33 forks source link

Keep the browser in sync with changed files #76

Closed JM-Mendez closed 4 years ago

JM-Mendez commented 4 years ago

Currently whenever files are updated, the server needs to be restarted and the browser reloaded.

Initially I chose webpack in order to use more modern tooling. This went well at first, but at around 2/3 of the way in I fell into issues with webpack/express and the jquery codebase.

At this point I realized that I had already updated a good portion of the site to use es6 import/export, and to continue further would mean a pretty comprehensive refactor.

Honestly I would love to refactor this app to React (preferably in typescript), and having been through the codebase I feel confident in accomplishing it.

But since this wasn't the target of the issue, I fell back to using gulp. Although with gulp we do lose hot module reloading as the entire application needs to be reloaded.

Description

EDIT:

After working on a separate issue, I realized that the express server is only being called on the main route. Therefore I was able to greatly simplify the setup by letting browserSync handle serving the files and updating on changes.

After this pull request, every file change in the ./views and ./src directories will:

1. recompile files 2. map hashed css files to bust the browser cache (rev-manifest.json) 3. restart the server 4. reload the browser

  1. compile pug files into a static html file
  2. compile and copy all other assets
  3. serve and update with browerSync

Related issues and discussion

67

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

JM-Mendez commented 4 years ago

Hi @AdrianBZG @Nikhil-Vats @amand1996. I'm an Outreachy May 2020 applicant. I noticed I couldn't request a reviewer, and wasn't sure whether to tag you on discord or here to inform you of the PR.

I'm assuming here is best. If not, can you let me know which is the preferred method?

Nikhil-Vats commented 4 years ago

Great work first of all. :+1: The feature works as intended. :smile: :rocket:

There are some changes required though -

  1. You should refactor your commits in a way so that the unnecessary changes in the first commit are not committed at all. Does that make sense? If you need me to elaborate, please ask.
  2. You should instead add one commit for every sub-feature you have added.
  3. I really appreciate the detailed commit messages you have added, they really helped me a lot but it would be better to add them as comments and change the commit messages. Following article can help you in doing that - https://github.com/erlang/otp/wiki/writing-good-commit-messages

Summarising, only commit the necessary changes. Don't commit unnecessary ones and change them in the following commits.

Nikhil-Vats commented 4 years ago

If you need any help, feel free to ask me. :smiley:

JM-Mendez commented 4 years ago

Hi @Nikhil-Vats thanks for the compliment. I'll reply to each individually:

  1. I can rebase my branch and purge those commits.

  2. I can def make a commit for each subfeature. Initially I squashed my commit history. The 2 commits that came after I didn't rebase and force push since I wasn't sure about your merge process and didn't want to ruin the history hashes.

  3. Are you specifically speaking about any portion of that post?

    a. For instance, I noticed that while my summary line was in the imperative mood, my description wasn't.

    b. also I'm not sure what you mean by adding the commit messages as comments. Do you mean to write less in the commit message and explain further in the PR comment section? I was under the impression that commit messages should be detailed so they can be viewed in the file's commit history (like in an IDE).

This Outreachy program is the first time I have ever had my code reviewed, so I greatly appreciate your time and guidance on this 😄

Nikhil-Vats commented 4 years ago

  1. Sure, you should definitely purge the commits with unnecessary changes that are overwritten in the following ones. (Merge/squash the commits so that only final changes are there in the commits in the PR)

  2. If you can quickly refactor the commits in a way such that there's a commit for each sub-feature - do it but we can also skip this step for this PR. Just make sure you do this for your future PRs.

  3. The commit messages should be detailed but you need not explain what you did in your previous commit that was wrong and so, you are changing that. Just explain the fixes made and why they were made.

More here. These are not hard and fast rules but you should try to follow them.

AdrianBZG commented 4 years ago

@JM-Mendez Very nice work! This has been awaited for a long time!

Merging 🚀