feat: add option to cache `pattern.transform` (`pattern.cache`) #176

alexander-akait · 2017-11-24T13:01:41Z

`Notable Changes`

implement serialize for cache key to invalidate cache after pattern.transform changed
add tests

michael-ciniawsky

Why not using memoization instead and make it the default for transform ?

michael-ciniawsky · 2017-11-24T19:36:24Z

src/writeFile.js

+                };
+
+                if (pattern.cache) {
+                    const cacheKey = pattern.cacheKey


Do we really need to ability to customize the cacheKey via pattern.options ?

@michael-ciniawsky yep, we can use package inside transform function which get options, to invalidate cache you should pass this variables as cacheKey

michael-ciniawsky · 2017-11-24T19:38:19Z

src/writeFile.js

+                if (pattern.cache) {
+                    const cacheKey = pattern.cacheKey
+                        ? pattern.cacheKey
+                        : JSON.stringify(serialize({


Could we use/generate an id or hash for patterns to avoid serialization ?

@michael-ciniawsky we can generate md5 or sha algo for this

@michael-ciniawsky after testing with difference arguments and platform, seems serialize is best solution, btw we can do this in uglify plugin

alexander-akait · 2017-12-05T12:24:37Z

@michael-ciniawsky to allow cache between webpack builds, we do this in uglify plugin

alexander-akait · 2017-12-05T14:30:37Z

@michael-ciniawsky friendly ping

michael-ciniawsky · 2017-12-05T18:49:14Z

src/writeFile.js

+                        globalRef.cacheDir = findCacheDir({ name: 'copy-webpack-plugin' });
+                    }
+
+                    const cacheKey = pattern.cacheKey


Do we really need pattern.cacheKey (Custom cacheKey) ? I'm not in favor of it, seems unnecessary

@michael-ciniawsky we need another opinion 😄

@michael-ciniawsky btw we can remove this option, because we use pattern as part of cache key and if you want invalidate cache you can use cache option (example cache: '/path/to/' + crypto.createHash('md5').update(JSON.stringify(something) || serialize(something)).digest("hex"))

@evilebottnawi If you add it to the cache pattern option itself { cache: { key: '...' } } || { cache: key } I'm ok with it, but adding to many (related) options 'top scope' of a pattern is unnecessary (bloat) and can be confusing for folks new to copy-webpack-plugin e.g if they assume pattern.cacheKey is mandatory to use for some reason (many don't fully read the docs). There are already too many options for patterns and most folks will tend to use simple patterns like { from: '', to: '' } || { from: '', to: '', cache: true } which is sufficient && straight forward for most of the plugin use cases.

@michael-ciniawsky btw, sometimes i have external variables for function inside in transofrm and i can't invalidate cache without this option, i can only enable or disable cache, but this not convenient. Example i use nunjucks inside transform, some template render long time and i want to cache this operation, also i know when i should invalidate cache and cacheKey solve this problem.

Yep, but this already goes under 'advanced' usage in the scope of copy-webpack-plugin imo and wouldn't this work with { cache: ${customCacheKey} instead of { cache: true, cacheKey: ${customCacheKey} } for some reason ?

michael-ciniawsky · 2017-12-05T18:50:37Z

src/writeFile.js

+                    const cacheKey = pattern.cacheKey
+                        ? pattern.cacheKey
+                        : serialize({
+                            name: name,


serialize({ name, version, pattern, content })

michael-ciniawsky · 2017-12-05T18:51:55Z

tests/index.js

+                .then(() => {
+                    return cacache
+                        .ls(cacheDir)
+                        .then((cacheEntriesList) => {


cacheEntriesList => cacheEntries

michael-ciniawsky · 2017-12-05T18:53:18Z

tests/index.js

+                    return cacache
+                        .ls(cacheDir)
+                        .then((cacheEntriesList) => {
+                            const cacheEntriesListKeys = Object.keys(cacheEntriesList);


cacheEntriesListKeys => cacheKeys

const cacheKeys = Object.keys(cacheEntries)

michael-ciniawsky · 2017-12-07T00:45:33Z

src/writeFile.js

+                        globalRef.cacheDir = findCacheDir({ name: 'copy-webpack-plugin' });
+                    }
+
+                    const cacheKey = pattern.cacheKey


@evilebottnawi If you add it to the cache pattern option itself { cache: { key: '...' } } || { cache: key } I'm ok with it, but adding to many (related) options 'top scope' of a pattern is unnecessary (bloat) and can be confusing for folks new to copy-webpack-plugin e.g if they assume pattern.cacheKey is mandatory to use for some reason (many don't fully read the docs). There are already too many options for patterns and most folks will tend to use simple patterns like { from: '', to: '' } || { from: '', to: '', cache: true } which is sufficient && straight forward for most of the plugin use cases.

michael-ciniawsky · 2017-12-07T00:49:41Z

tests/index.js

+
+                            expect(cacheKeys).to.have.lengthOf(1);
+
+                            cacheKeys.forEach((serializedCacheEntry) => {


cacheKeys.forEach((cacheKey) => { // eslint-disable-next-line no-eval cacheKey = eval(`(${cacheKey})`); expect(cacheKey.pattern.from).to.equal(from); });

@michael-ciniawsky done

alexander-akait · 2017-12-12T12:14:11Z

@michael-ciniawsky friendly ping, move key inside cache option.

michael-ciniawsky

👍

alexander-akait added priority: 5 (nice to have) pr_action: Review semver: Minor type: Feature labels Nov 24, 2017

alexander-akait requested review from michael-ciniawsky and joshwiens November 24, 2017 13:01

alexander-akait force-pushed the feature-cache branch 4 times, most recently from 920de2a to 8ef8d5b Compare November 24, 2017 14:19

michael-ciniawsky requested changes Nov 24, 2017

View reviewed changes

michael-ciniawsky added this to the 4.3.0 milestone Nov 24, 2017

alexander-akait force-pushed the feature-cache branch 7 times, most recently from 38e700c to 7c0e7c4 Compare December 5, 2017 14:28

alexander-akait changed the title ~~[WIP] feat: option for caching transform function~~ feat: option for caching transform function Dec 5, 2017

michael-ciniawsky requested changes Dec 5, 2017

View reviewed changes

alexander-akait force-pushed the feature-cache branch from 7c0e7c4 to 5c849c2 Compare December 6, 2017 12:55

michael-ciniawsky reviewed Dec 7, 2017

View reviewed changes

alexander-akait force-pushed the feature-cache branch from 5c849c2 to 8969493 Compare December 7, 2017 10:26

alexander-akait closed this Dec 12, 2017

alexander-akait reopened this Dec 12, 2017

feat: option for caching transform function

48c19ff

alexander-akait force-pushed the feature-cache branch from 8969493 to 48c19ff Compare December 12, 2017 12:13

michael-ciniawsky approved these changes Dec 12, 2017

View reviewed changes

michael-ciniawsky added status: Approved and removed priority: 5 (nice to have) labels Dec 12, 2017

michael-ciniawsky changed the title ~~feat: option for caching transform function~~ feat: add option to cache pattern.transform (pattern.cache) Dec 12, 2017

michael-ciniawsky removed the pr_action: Review label Dec 14, 2017

michael-ciniawsky merged commit 20c143b into master Dec 14, 2017

michael-ciniawsky deleted the feature-cache branch December 14, 2017 04:24

michael-ciniawsky removed this from the 4.3.0 milestone Dec 14, 2017

dependencies bot mentioned this pull request Dec 14, 2017

Update copy-webpack-plugin in /site from 4.2.3 to 4.3.0 src-zone/material#158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add option to cache `pattern.transform` (`pattern.cache`) #176

feat: add option to cache `pattern.transform` (`pattern.cache`) #176

alexander-akait commented Nov 24, 2017 •

edited by michael-ciniawsky

Loading

michael-ciniawsky left a comment

michael-ciniawsky Nov 24, 2017

alexander-akait Nov 28, 2017

michael-ciniawsky Nov 24, 2017

alexander-akait Nov 28, 2017

alexander-akait Dec 5, 2017

alexander-akait commented Dec 5, 2017

alexander-akait commented Dec 5, 2017

michael-ciniawsky Dec 5, 2017

alexander-akait Dec 6, 2017

alexander-akait Dec 6, 2017

michael-ciniawsky Dec 7, 2017

alexander-akait Dec 7, 2017

michael-ciniawsky Dec 7, 2017

michael-ciniawsky Dec 5, 2017

michael-ciniawsky Dec 5, 2017

michael-ciniawsky Dec 5, 2017

michael-ciniawsky Dec 7, 2017

michael-ciniawsky Dec 7, 2017

alexander-akait Dec 7, 2017

alexander-akait commented Dec 12, 2017

michael-ciniawsky left a comment


		expect(cacheKeys).to.have.lengthOf(1);

		cacheKeys.forEach((serializedCacheEntry) => {

feat: add option to cache pattern.transform (pattern.cache) #176

feat: add option to cache pattern.transform (pattern.cache) #176

Conversation

alexander-akait commented Nov 24, 2017 • edited by michael-ciniawsky Loading

Notable Changes

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait commented Dec 5, 2017

alexander-akait commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait commented Dec 12, 2017

michael-ciniawsky left a comment

Choose a reason for hiding this comment

feat: add option to cache `pattern.transform` (`pattern.cache`) #176

feat: add option to cache `pattern.transform` (`pattern.cache`) #176

alexander-akait commented Nov 24, 2017 •

edited by michael-ciniawsky

Loading

`Notable Changes`