feat: etag support #746

privatenumber · 2020-10-27T07:46:03Z

This PR contains a:

Motivation / Use-Case

Etags: ETag support #738

Breaking Changes

N/A

Additional Info

Will work on tests tomorrow but opening a PR to get a head start on review and feedback.

codecov · 2020-10-27T10:29:32Z

Codecov Report

Merging #746 (420c6fb) into master (1df8477) will decrease coverage by 6.93%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master     #746      +/-   ##
==========================================
- Coverage   99.15%   92.21%   -6.94%     
==========================================
  Files          10       10              
  Lines         236      257      +21     
  Branches       72       78       +6     
==========================================
+ Hits          234      237       +3     
- Misses          2       16      +14     
- Partials        0        4       +4

Impacted Files	Coverage Δ
src/middleware.js	`67.85% <14.28%> (-32.15%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1df8477...420c6fb. Read the comment docs.

alexander-akait · 2020-10-27T12:08:54Z

src/middleware.js

+    compiler.hooks.done.tap('webpack-dev-middleware', computeEtags);
+  } else {
+    compiler.plugin('done', computeEtags);
+  }


No hooks here

We already can load assets here, please look at logic

I saw that there's access to the memfs instance, but is there a manifest object (eg. build stats) I can iterate over to get all the asset paths?

Why you need iterate?

Anyway if you need this, you have context, put this logic inside done hooks and collect them context.etags

Sounds good.

I want to iterate a list of distribution files to efficiently calculate etags. If I only have the memfs instance, I'd have to traverse the entire fs, which isn't optimal.

alexander-akait · 2020-10-27T12:09:31Z

src/middleware.js

 export default function wrapper(context) {
+  const { compiler } = context;
+  const etagRegistry = new Map();


memory leak

We need to WeakMap and use assets source as key

AFAIK, Weakmaps don't allow primitives (eg. strings) to be used as keys.

I'll clear all entries via Map#clear at the beginning of computeEtags.

No, it is still memory leak, you should not use primitives for weak map, you have asset object for this

Resolved the other thread about this so all conversation is in this thread.

Can you explain why there would still be a memory leak if unused etags are being cleaned up per build?

Can you explain why there would still be a memory leak if unused etags are being cleaned up per build?

Because we create etagRegistry on middleware start so next compilation will put new files again and again, and if you use name.[contenthash][ext] you will increase size infinitely, it will be difficult to debug, but this is a potential leak, using WeakMap help to solve it, when asset was removed from comilation.assets it will be removed from WeakMap, so we free memory

I see, thanks for explaining.

That's addressed by etagRegistry.clear() on every build completion because .clear() removes all entries in a Map.

I'm wondering how you imagine using a WeakMap is possible? I would jump to using it if it were possible to get the asset object (they key) when a new HTTP request comes in here.

That's addressed by etagRegistry.clear() on every build completion because .clear() removes all entries in a Map.

it was reduce performance, we should avoid it

I'm wondering how you imagine using a WeakMap is possible? I would jump to using it if it were possible to get the asset object (they key) when a new HTTP request comes in here.

store source of assets and use it as key

alexander-akait · 2020-10-27T12:10:33Z

src/middleware.js

@@ -49,6 +75,19 @@ export default function wrapper(context) {
        return;
      }

+      const assetEtag = etagRegistry.get(filename);
+      if (assetEtag) {


We need option to enable/disable it, in future we can enable it by default

src/middleware.js

alexander-akait

Also we have immutable flag in assetInfo, this flag assets will do be changed never

privatenumber · 2020-10-29T07:03:11Z

As I work on the tests, I'm just realizing that express adds etags by default.

Feels redundant to make it disabled by default on the middleware, and this also makes it difficult to test because etags are on even if the middleware option is disabled.

Do you mind if I make this a PR against #747? That way I can pull in the vanilla HTTP version and use connect in the test.

alexander-akait · 2020-10-29T11:40:13Z

As I work on the tests, I'm just realizing that express adds etags by default.

Yes, we will enable it by default, but we really need a option to disable this

Do you mind if I make this a PR against #747? That way I can pull in the vanilla HTTP version and use connect in the test.

Yes

alexander-akait · 2020-11-13T16:13:03Z

I studied all the material and other implementation, so I want to implement options:

cacheControl
etag
lastModified

It should be not hard, WIP on this, but I think release will be on the next week only

alexander-akait · 2020-11-13T16:13:18Z

Anyway thanks for good idea

privatenumber · 2020-11-13T19:53:37Z

@evilebottnawi

I thought about time-based caching (cache-control/if-modified-since) as well but concluded it wouldn't be as useful as a content-based one (etag) in a rapid development environment.

I imagine it would actually lead to:

unexpected behavior (changed code not getting immediately fetched)
users not understanding the feature and disabling caching in the browser
people turning it off/not using it

alexander-akait · 2020-11-16T11:47:39Z

I will provide some docs on this to help developers, anyway etag is very easy to implement, so it will be implemented in near future and enabled by default

privatenumber · 2021-08-03T21:37:07Z

src/middleware.js

+        for (const assetId in assets) {
+          if (hasOwnProperty.call(assets, assetId)) {
+            const { existsAt: fsPath } = assets[assetId];
+            const etag = getETag(assets[assetId].source());


Seems this is no longer possible in Webpack 5, due to error:

[webpack-dev-middleware] Error: Content and Map of this Source is not available (only size() is supported)
at SizeOnlySource._error (/node_modules/webpack-sources/lib/SizeOnlySource.js:16:10)
at SizeOnlySource.source (/node_modules/webpack-sources/lib/SizeOnlySource.js:26:14)

@alexander-akait Do you have a recommended work around for this?

I ended up leveraging chunkHashs, which is actually more convenient because I can just use the hash as the etag instead of generating one from the file source:

const { chunkHashs: assetHashes } = stats.compilation.records; for (const assetId in assetHashes) { if (hasOwnProp(assetHashes, assetId)) { etagRegistry.set(assetId, JSON.stringify(assetHashes[assetId])); } }

If there's an alternative you recommend, LMK.

/cc @sokra What do you think?

privatenumber · 2021-08-03T21:39:37Z

src/middleware.js

+        const { assets } = stats.compilation;
+        for (const assetId in assets) {
+          if (hasOwnProperty.call(assets, assetId)) {
+            const { existsAt: fsPath } = assets[assetId];


Seems existsAt doesn't exist in Webpack 5 either

privatenumber added 7 commits October 27, 2020 02:32

temporariy disable files

eee8640

temporariy disable files

768d0c0

refactor: use native req/res methods

bf79de7

feat: added etag support

6a28828

revert: pkg files

203c767

style: added eslint-ignore for res statusCode

7aca742

test: fixed req/res methods

494728c

privatenumber requested review from alexander-akait and hiroppy as code owners October 27, 2020 07:46

privatenumber changed the title ~~feat: Etag support~~ feat: etag support Oct 27, 2020

revert: refactor

a2b418d

revert: eslint-disable

0129e2c

alexander-akait reviewed Oct 27, 2020

View reviewed changes

src/middleware.js Outdated Show resolved Hide resolved

alexander-akait reviewed Oct 27, 2020

View reviewed changes

privatenumber added 3 commits October 27, 2020 18:57

fix: memory leak in etagRegistry

c9f954f

feat: etag option

1bd3270

refactor: removed compiler variable

420c6fb

privatenumber mentioned this pull request Oct 29, 2020

refactor: use native req/res methods #747

Closed

6 tasks

privatenumber commented Aug 3, 2021

View reviewed changes

alexander-akait mentioned this pull request Mar 28, 2024

feat: etag support #1797

Merged

7 tasks

alexander-akait closed this in #1797 Mar 28, 2024

Uh oh!

feat: etag support #746

feat: etag support #746

Uh oh!

Conversation

privatenumber commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation / Use-Case

Breaking Changes

Additional Info

Uh oh!

codecov bot commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

privatenumber Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

privatenumber Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

privatenumber commented Oct 29, 2020

Uh oh!

alexander-akait commented Oct 29, 2020

Uh oh!

alexander-akait commented Nov 13, 2020

Uh oh!

alexander-akait commented Nov 13, 2020

Uh oh!

privatenumber commented Nov 13, 2020

Uh oh!

alexander-akait commented Nov 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

privatenumber commented Oct 27, 2020 •

edited

Loading

codecov bot commented Oct 27, 2020 •

edited

Loading

privatenumber Oct 27, 2020 •

edited

Loading

alexander-akait Oct 28, 2020 •

edited

Loading

alexander-akait Oct 27, 2020 •

edited

Loading

privatenumber Oct 27, 2020 •

edited

Loading

alexander-akait Oct 28, 2020 •

edited

Loading

alexander-akait Oct 27, 2020 •

edited

Loading