Webpack content hash

[HashTalmiz]

Description of proposed changes

What is the goal of this pull request? What does this pull request change?
To enable Cache busting.

Related issue(s)

Issue #1037, Added the features but still isn't working
Fixes #

Related to #

Testing

What steps should be taken to test the changes you've proposed?
If you added or changed behavior in the codebase, did you update the tests, or do you need help with this?

Thank you for contributing to Nextstrain!

[eharkins]

Hey @HashTalmiz thanks for this! I think the reason for failing the tests is just a simple typo of the imported HtmlWebpackPlugin here.

Things like this should be caught by running eslint on js files, which only happens for files in the src dir when you run npm lint, so you'll have to run npx eslint webpack.config.js to get it to run on that file (we aim to include more files - like the webpack ones you are developing - in this built-in linting command soon).

This will also make the diff in the PR just show the important places where you've changed the code, so we can review and merge :) Let us know if you have any issues running linting. Thanks!

[HashTalmiz]

Hey @eharkins Thanks for looking into this. That typo was pretty dumb on my part lol. Installed ESLint and Linted the webpack.config file and its ready for a push with the typo fix :P
One question though, as mentioned here (#1037 (comment)), should I remove the other references and just have it hash to index.html OR keep what I've written and just fix the typos?

In other words, Is this required,

const HTMLdependentOnHash = [
		new HtmlWebpackPlugin({
			filename : 'index.html',
			template : './index.html'
		}),
		new HtmlWebpackPLugin({                   //fixed typo
			filename : 'requests.html',
			template : './docs/customise-client/requests.html'
		}),
		new HtmlWebpackPLugin({                   //fixed typo
			filename : 'services.html',
			template : './src/template/services.pug'
		}),
		new HtmlWebpackPLugin({                   //fixed typo
			filename : 'requestIndex.html',
			template : './docs/customise-client/requests/index.html'
		})
	];

or just this?

const HTMLdependentOnHash = [
		new HtmlWebpackPlugin({
			filename : 'index.html',
			template : './index.html'
		})
];

[eharkins]

Yes, @HashTalmiz I think just the one HtmlWebpackPlugin should be necessary for index.html. For that one, is there a reason you specify explicitly the filename : 'index.html' (I think it is the default) or was that just to differentiate from the others? Will it end up in the webpack output directory dist like the example in the HtmlWebpackPlugin docs if you specify it explicitly like this?

I'm assuming the two entries that reference files in docs are because those files referenced the bundle file, but it looks like that was just listing the name of that file in the docs and not important for webpack, so I don't think they are needed in the webpack plugin.

What about this one, what does it do?

		new HtmlWebpackPLugin({                   //fixed typo
			filename : 'services.html',
			template : './src/template/services.pug'

Sorry for not getting to the actual content of your PR before. Go ahead and commit the linting so we can have a clean diff to look at. Thanks!

[HashTalmiz]

Yeah, the filename property decides what it'll be called in dist. I gave filenames to differentiate it from the other emitted files since I thought there would be more than one. But that isn't going to be the case. So here's where I am:

After a final Lint of webpack.config.js (--fix), I ran npm run dev, and was faced with this error (at the bottom):

So I did what it said. Running on dev again worked! So I did npm run build and:

You can see the hashed auspice.bundle.js file in the first line and the rest of the chunks being hashed too!!

It ended with a warning:

I'll commit the changes for you to see all the changes (and see if anything else is messed up by Prettier [my bad lol, I turned it off] )

[HashTalmiz]

@eharkins Well, this pull request was made a while ago and I guess a few new packages were added since then. Also I made changes on the master immediately after the fork. How you handle merge conflicts?

Note: I had a look at this tool

[eharkins]

@HashTalmiz in terms of conflicts, it should work to rebase your branch on auspice/master.

In terms of the diff, it still seems like there are formatting changes to both files in the PR; after rebasing can you make sure that all of those are reverted so that it's just the changes relating to the html webpack plugin in the PR? Let me know if you have any trouble, thanks!

[HashTalmiz]

@eharkins So I've done my best in getting the merge conflicts fixed and did a rebase to the current upstream master. The CI still fails for some reason. I found this article on a similar issue locally. But I have no idea how I can fix this. Maybe @jameshadfield can help out here?

[eharkins]

@HashTalmiz this line (and many others) that are supposedly changed in the PR have the exact same content as in master, which makes me think that something (perhaps the editor you're using) is adding different line ending characters (or something else invisible like tabs vs spaces) to all the lines that you edit (including the ones you merged) so that they show up different in diffs even though nothing has really changed with them.

Maybe you can test this by doing local git diff after saving with different editors (though you may have to explicitly remove special line ending characters if they are in there now).

[HashTalmiz]

The thing is, I haven't made any changes manually to any of these files for my editor to do anything (Except for webpack.config and package.json) All of these were bought by the rebase which I tried to do with the master branch

[eharkins]

With respect to the npm ci command in the CI, it looks like it has to do with package.json being out of sync with package-lock.json.

Did you run npm install to update the package-lock after adding the html webpack plugin dependency?

[HashTalmiz]

Did you run npm install to update the package-lock after adding the html webpack plugin dependency?

I didn't. But when I did hit npm install or npm update or this as suggested in stackoverflow https://stackoverflow.com/questions/57867267/how-to-update-package-lock-json-without-doing-npm-install nothing changed. Nothing was added to lock.json, therefore nothing to commit, which is the reason why this seems bizzare :/

[HashTalmiz]

It wasn't white spaces or tabs, It was the date format, could it have been Git conforming to my format? Could you have a look at package.json and package-lock.json and see if there's something wrong there?

[HashTalmiz]

@HashTalmiz this line (and many others) that are supposedly changed in the PR have the exact same content as in master, which makes me think that something (perhaps the editor you're using) is adding different line ending characters (or something else invisible like tabs vs spaces) to all the lines that you edit

@eharkins I think you are referring to things like this? I almost agreed with you there but then I realized, every single one of these Lookalikes have the Date format changed!
For instance, look here, at the start of the second line on both sides
3/24/2020 on the left and 24/03/2020 on the right
I'm not sure how this came to be. I'm 100% sure I haven't opened these files for my editor to do this. Should I remove these from the PR right from github?

[eharkins]

This change is from master

[HashTalmiz]

@eharkins Btw I remember saying "yes" to a prompt saying something about adding Git LFS. Since then I've had these unwanted files coming up as unstaged commits. Could this be causing all the differences? I'm sorry if this seems unrelated/if you have no idea what this is.

[eharkins]

@HashTalmiz Git LFS is a new dependency you need to install which is happening now because you rebased on master.

I think all the issues here come from the rebase and merge. Maybe you can get just your webpack related changes back from the reflog without this commit that changes all the other files and (re-)add them on top of a clean master?

I think this'll mean manually remaking the changes or add the commits with rebase and a clean merge. This may help https://git-scm.com/book/en/v2/Git-Branching-Rebasing.

Sorry for all the trouble here and thanks for your patience!

[HashTalmiz]

@eharkins Okay, so I reverted back to the best commit I've had which is where this PR is right now. Tried to do a fresh rebase after reading up all the info needed and there's where I found the problem.

All the files were fine before that last checkout command. Once I did that, all those unwanted & weird changes came back into the local repo on the master branch(which was clean thus far). It failed to checkout and this is what was added:

(Notice the .eslint file change in the first line. This was the exact change you reviewed. All of this was in the last commit that turned things ugly)
I did a git reset --hard and a git clean to get back and tried it again. And the same thing happened when I did a rebase and merge! So now I can't even do a clean rebase or merge but I know the problem is related to git lfs. I don't have Git LFS installed locally, not sure what'll come up if I do. I'm considering just giving up at this point. 😞

[tsibley]

@HashTalmiz Regarding the git lfs error you're seeing, I think it's the same issue as discussed and solved here: #1089 (comment)

[HashTalmiz]

Hey @tsibley, Thanks for that reference; got rid of the LFS in my global .config, I no longer get that error but for some reason when I do a git pull from auspice/master, I end up getting all the unwanted file changes (which aren't a part of master) exactly like last time. I'm not sure if its just me or you guys are facing this too. Can you guys try it out and let me know?

But anyway, I found a workaround with $ git checkout --track [remote]/master for a clean copy of aupsice/master, and did the rebase required.
@eharkins You can have a look at the Files changed now. I do apologize for the change of all double quotes to single quotes in webpack.config.js .It was from Prettier's JS standards from my editor, but I hope that isn't a problem. The indentation from tabs to spaces has been fixed so there's no merge confict in package.json

Edit: Some checks have failed. Not sure why but I know that the errors aren't because of the changes.
Edit 2: Updating dependencies didn't fix it, in fact, it made it worse. Reverting commit.

[eharkins]

@HashTalmiz I added your changes to the webpack config cleanly onto master in my fork, and was hoping to be able to use that to merge this work (while still attributing it to you).

However, upon testing it out with Auspice customizations which we rely on for nextstrain.org, I discovered some issues (described here) that would need resolving before we could use the added webpack functionality in Auspice, since keeping nextstrain.org up and running is a priority for us right now.

[HashTalmiz]

@eharkins Responded here: eharkins@4741ed4#r39276548

[jameshadfield]

Thanks both @HashTalmiz and @eharkins for your work here, it's really appreciated. I've just read through #1126 and believe that solution to be more far-reaching than this PR and thus am going to close this one. Please reopen if there are features here not implemented by #1126.