Support for an advanced “persistentCache” by martinheidegger · Pull Request #124 · browserify/module-deps

martinheidegger · 2017-01-26T20:56:16Z

There are a few implementations of persistent caches for browserify such as persistify or browserify-incremental. But looking at each implementation it seemed like they struggle with the interface given by module-deps to cache: which is just an object lookup. This PR adds an additional cache interface to module-deps that should make it possible to implement not just resource-efficient but also less hacky caching solutions for browserify.

…utside of the current memory.

terinjokes · 2017-01-26T21:18:46Z

readme.markdown

+        }
+        if (isInCache()) {
+            return cb(null, {
+                source: fs.readFileSync(file, 'utf8'), // String of the fully processed file


Since this is the "in cache" case, the example should probably show the contents coming from somewhere other than fs.

It does now.

terinjokes · 2017-01-26T21:20:09Z

readme.markdown

+        // optional (can be null) stream that provides the data from
+        // the hard disk, can be provided in case the file data is used
+        // to evaluate the cache identifier
+        stream = fs.createReadStream(file)


Can there be a better example here, since this is what I expect fallback to be doing.

I updated the example. it now uses a db.

terinjokes · 2017-01-26T21:21:17Z

index.js

-                fromSource(file, body.toString('utf8'), pkg, fakePath);
-            }))
-        ;
+        self.persistentCache(file, id, pkg, function fallback (stream, cb) {


Is it possible to move the fallback function somewhere other than an inline parameter here?

Moved it to an outside scope.

- Used database instead of filesystem in the cache example. - Moved the fallback handler to a method one scope out.

martinheidegger · 2017-01-27T00:06:39Z

@terinjokes Thank you for your review! I updated the PR. One thing to mention: I changed the implementation. Now the fallback takes an optional string instead of a stream. This change might seem odd, I came to the conclusion that this would be the better API when I tried to implement a cache. The cache could read the source in order to create a Key but if it did that with streams (i.e. to create a hash) then it would need to keep all the file data in memory and create yet another stream off of the in-memory data which would just make the impl. more complex without a real win for the module-deps.

martinheidegger · 2017-01-29T19:18:56Z

@terinjokes Thank you for approving the PR, how good are the chances for a merge?

terinjokes · 2017-01-30T19:02:47Z

I'd prefer to have someone else take a look first.

martinheidegger · 2017-01-30T22:38:16Z

Whom would be good to ask?

martinheidegger · 2017-02-01T01:37:53Z

Looked into the npm owners and pinging @substack @zertosh @jmm @MellowMelon @ahdinosaur for a review.

ahdinosaur

hi hi hi

ahdinosaur · 2017-02-01T03:04:30Z

index.js

    return true;
 }

+function toStream (dataAsString) {


nice refactor 👍

ahdinosaur · 2017-02-01T03:06:03Z

index.js

+        }
    });

+    function getDeps (file, src) {


more goods 🐰

ahdinosaur · 2017-02-01T03:06:40Z

index.js


    this.basedir = opts.basedir || process.cwd();
+    this.persistentCache = opts.persistentCache || function (file, id, pkg, fallback, cb) {
+        setImmediate(function () {


why setImmediate here?

͓̞̦̼͎. ̖̱̗̝͉͈Z̧̬͎̥͕̻̜AḺ̡̼G̝͓͘O̴̫̹̰̯͕!͈̱̟͈̕

Edit: let me extend: Since the implementation expects an async handler it felt only appropriate to uphold the contract in the default implementation

of course, thanks!

ahdinosaur · 2017-02-01T03:08:44Z

index.js

+
+        function persistentCacheFallback (dataAsString, cb) {
+            var stream
+            if (dataAsString) {


ternary? also semicolon styles missing here;

ahdinosaur · 2017-02-01T03:10:04Z

index.js

+                            source: src,
+                            package: pkg,
+                            deps: deps.reduce(function (deps, dep) {
+                                deps[dep] = true


if (pedantic);

ahdinosaur · 2017-02-01T03:12:41Z

readme.markdown

+        // then you can pass `null` instead of the source and the fallback will
+        // fetch the data by itself.
+        //
+        fallback(fileData, function (error, cacheableEntry) {


where did fallback come from here? is missing from above description of function arguments.

ahdinosaur · 2017-02-01T03:18:34Z

test/cache_persistent.js

+    p.on('error', function (err) { t.equals(err.message, 'foo') });
+});
+
+test('allow passing of a different stream', function (t) {


what stream? do you mean a different string to fallback?

Yes, it used to be a stream :)

Fixes: browserify#124 (comment)

martinheidegger · 2017-02-01T04:17:12Z

Thanks for the review @ahdinosaur! Pushed a few fixes to the PR in regards to the linting.

ahdinosaur

ya ya ya

martinheidegger · 2017-02-01T04:31:16Z

Two approved reviews 😃

martinheidegger · 2017-02-02T04:35:22Z

@terinjokes @ahdinosaur Are the chances of a merge higher now with two Reviews? 😅

martinheidegger · 2017-02-03T16:07:12Z

Note: I published an implementation of persistentCache here: https://github.com/martinheidegger/browserify-persist-fs

ghost · 2017-02-14T02:31:59Z

Thanks everyone for reviewing this and going back and forth. Published as 4.1.0.

ghost · 2017-02-14T02:33:30Z

We should also ping @jsdf and @royriojas upstream to make sure they know about this patch so they can make the best use of these changes.

using https://github.com/martinheidegger/browserify-persist-fs from browserify/module-deps#124

ahdinosaur · 2017-03-20T04:17:02Z

@martinheidegger hey, so the persistent cache breaks when using bulk-require and bulkify. specifically when you create a new file in your bulk require glob, it is ignored and built as before. i see you tried to fix file events with #127, but as far as i can tell it's still broken when using that fix. any ideas?

Support for an advanced “persistentCache” to be used for persisting o…

6faac31

…utside of the current memory.

terinjokes suggested changes Jan 26, 2017

View reviewed changes

martinheidegger added 2 commits January 27, 2017 08:50

Typo

ef4532a

- Switched from passing on a stream to passing on a string.

37ca0bb

- Used database instead of filesystem in the cache example. - Moved the fallback handler to a method one scope out.

terinjokes approved these changes Jan 27, 2017

View reviewed changes

ahdinosaur reviewed Feb 1, 2017

View reviewed changes

martinheidegger added 4 commits February 1, 2017 13:09

Fixed browserify#124 (comment)

6390913

Fixes browserify#124 (comment)

c3ac60f

Adds missing fallback document

d28fee1

Fixes: browserify#124 (comment)

Properly named the test for the string passing

c940a1c

Fixes: browserify#124 (comment)

ahdinosaur approved these changes Feb 1, 2017

View reviewed changes

ghost merged commit c940a1c into browserify:master Feb 14, 2017

martinheidegger deleted the feature/persistentCache branch February 14, 2017 02:39

ahdinosaur added a commit to root-systems/catstack that referenced this pull request Mar 1, 2017

implement assets directly without bankai and with caching

fb17ac1

using https://github.com/martinheidegger/browserify-persist-fs from browserify/module-deps#124

ahdinosaur mentioned this pull request Mar 30, 2017

better browserify root-systems/catstack#108

Open

goto-bus-stop mentioned this pull request Mar 9, 2018

Readable streams can be passed to opts.fileCache #121

Open

snyk-bot mentioned this pull request Mar 7, 2020

[Snyk] Security upgrade module-deps from 3.9.1 to 5.0.0 fishbar/nexe#3

Open

This pull request was closed.

Conversation

martinheidegger commented Jan 26, 2017

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!

Choose a reason for hiding this comment

Uh oh!

martinheidegger commented Jan 27, 2017

Uh oh!

martinheidegger commented Jan 29, 2017

Uh oh!

terinjokes commented Jan 30, 2017

Uh oh!

martinheidegger commented Jan 30, 2017

Uh oh!

martinheidegger commented Feb 1, 2017

Uh oh!

ahdinosaur left a comment

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!

martinheidegger Feb 1, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinheidegger commented Feb 1, 2017

Uh oh!

ahdinosaur left a comment

Choose a reason for hiding this comment

Uh oh!

martinheidegger commented Feb 1, 2017

Uh oh!

martinheidegger commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinheidegger commented Feb 3, 2017

Uh oh!

ghost commented Feb 14, 2017

Uh oh!

ghost commented Feb 14, 2017

Uh oh!

ahdinosaur commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

martinheidegger Feb 1, 2017 •

edited

Loading

martinheidegger commented Feb 2, 2017 •

edited

Loading

ahdinosaur commented Mar 20, 2017 •

edited

Loading