Always use absolute paths after reading from cache (#83)

* chore(na): change default cache directory * chore(na): remove error changes * fix(na): remove contextual relative info from watchable paths * fix(na): always use absolute paths after reading from cache * test(na): add test for require absolute pahts * test(na): added correct cache context * test(na): fix all the tests * test(na): fix all the tests * chore(na): add development note about pathWithContext behaviour
webpack-contrib · Jun 25, 2019 · 9d93c31 · 9d93c31
1 parent badc3e9
commit 9d93c31
Show file tree

Hide file tree

Showing 7 changed files with 110 additions and 11 deletions.
diff --git a/package.json b/package.json
@@ -72,6 +72,7 @@
     "npm-run-all": "^4.1.5",
     "prettier": "^1.17.1",
     "standard-version": "^6.0.1",
+    "uuid": "^3.3.2",
     "webpack": "^4.33.0",
     "webpack-cli": "^3.3.2"
   },

diff --git a/src/index.js b/src/index.js
@@ -53,6 +53,10 @@ function roundMs(mtime, precision) {
   return Math.floor(mtime / precision) * precision;
 }
 
+// NOTE: We should only apply `pathWithCacheContext` transformations
+// right before writing. Every other internal steps with the paths
+// should be accomplish over absolute paths. Otherwise we have the risk
+// to break watchpack -> chokidar watch logic  over webpack@4 --watch
 function loader(...args) {
   const options = Object.assign({}, defaults, getOptions(this));
   validateOptions(schema, options, 'Cache Loader');
@@ -120,7 +124,10 @@ function loader(...args) {
       writeFn(
         data.cacheKey,
         {
-          remainingRequest: data.remainingRequest,
+          remainingRequest: pathWithCacheContext(
+            options.cacheContext,
+            data.remainingRequest
+          ),
           dependencies: deps,
           contextDependencies: contextDeps,
           result: args,
@@ -134,6 +141,10 @@ function loader(...args) {
   );
 }
 
+// NOTE: We should apply `pathWithCacheContext` transformations
+// right after reading. Every other internal steps with the paths
+// should be accomplish over absolute paths. Otherwise we have the risk
+// to break watchpack -> chokidar watch logic  over webpack@4 --watch
 function pitch(remainingRequest, prevRequest, dataInput) {
   const options = Object.assign({}, defaults, getOptions(this));
 
@@ -151,14 +162,20 @@ function pitch(remainingRequest, prevRequest, dataInput) {
   const callback = this.async();
   const data = dataInput;
 
-  data.remainingRequest = pathWithCacheContext(cacheContext, remainingRequest);
+  data.remainingRequest = remainingRequest;
   data.cacheKey = cacheKeyFn(options, data.remainingRequest);
   readFn(data.cacheKey, (readErr, cacheData) => {
     if (readErr) {
       callback();
       return;
     }
-    if (cacheData.remainingRequest !== data.remainingRequest) {
+
+    // We need to patch every path within data on cache with the cacheContext,
+    // or it would cause problems when watching
+    if (
+      pathWithCacheContext(options.cacheContext, cacheData.remainingRequest) !==
+      data.remainingRequest
+    ) {
       // in case of a hash conflict
       callback();
       return;
@@ -167,7 +184,14 @@ function pitch(remainingRequest, prevRequest, dataInput) {
     async.each(
       cacheData.dependencies.concat(cacheData.contextDependencies),
       (dep, eachCallback) => {
-        FS.stat(dep.path, (statErr, stats) => {
+        // Applying reverse path transformation, in case they are relatives, when
+        // reading from cache
+        const contextDep = {
+          ...dep,
+          path: pathWithCacheContext(options.cacheContext, dep.path),
+        };
+
+        FS.stat(contextDep.path, (statErr, stats) => {
           if (statErr) {
             eachCallback(statErr);
             return;
@@ -182,7 +206,7 @@ function pitch(remainingRequest, prevRequest, dataInput) {
           }
 
           const compStats = stats;
-          const compDep = dep;
+          const compDep = contextDep;
           if (precision > 1) {
             ['atime', 'mtime', 'ctime', 'birthtime'].forEach((key) => {
               const msKey = `${key}Ms`;

diff --git a/test/__snapshots__/compare-option.test.js.snap b/test/__snapshots__/compare-option.test.js.snap
@@ -0,0 +1,5 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`compare option should call compare with contextualized dep: errors 1`] = `Array []`;
+
+exports[`compare option should call compare with contextualized dep: warnings 1`] = `Array []`;
diff --git a/test/cacheContext-option.test.js b/test/cacheContext-option.test.js
@@ -73,8 +73,14 @@ const buildCacheLoaderCallsData = (calls, normalizePaths = true) =>
   ).sort(sortData);
 
 describe('cacheContext option', () => {
+  beforeEach(() => {
+    mockCacheLoaderWriteFn.mockClear();
+  });
+
   it('should generate relative paths to the project root', async () => {
     const testId = './basic/index.js';
+    await webpack(testId, mockBaseWebpackConfig);
+    mockCacheLoaderWriteFn.mockClear();
     const stats = await webpack(testId, mockRelativeWebpackConfig);
 
     const cacheLoaderCallsData = buildCacheLoaderCallsData(
@@ -83,7 +89,8 @@ describe('cacheContext option', () => {
 
     expect(
       cacheLoaderCallsData.every(
-        (call) => !call.remainingRequest.includes(path.resolve('.'))
+        (call) =>
+          !call.remainingRequest.includes(normalizePath(path.resolve('.')))
       )
     ).toBeTruthy();
     expect(BJSON.stringify(cacheLoaderCallsData, 2)).toMatchSnapshot(
@@ -95,6 +102,7 @@ describe('cacheContext option', () => {
 
   it('should generate non normalized relative paths to the project root on windows', async () => {
     const testId = './basic/index.js';
+    await webpack(testId, mockBaseWebpackConfig);
     await webpack(testId, mockRelativeWebpackConfig);
 
     const cacheLoaderCallsData = buildCacheLoaderCallsData(
@@ -123,6 +131,8 @@ describe('cacheContext option', () => {
 
   it('should generate absolute paths to the project root', async () => {
     const testId = './basic/index.js';
+    await webpack(testId, mockRelativeWebpackConfig);
+    mockCacheLoaderWriteFn.mockClear();
     const stats = await webpack(testId, mockBaseWebpackConfig);
 
     const cacheLoaderCallsData = buildCacheLoaderCallsData(
@@ -131,15 +141,16 @@ describe('cacheContext option', () => {
 
     expect(
       cacheLoaderCallsData.every((call) =>
-        call.remainingRequest.includes(path.resolve('.'))
+        call.remainingRequest.includes(normalizePath(path.resolve('.')))
       )
-    ).toBeFalsy();
+    ).toBeTruthy();
     expect(stats.compilation.warnings).toMatchSnapshot('warnings');
     expect(stats.compilation.errors).toMatchSnapshot('errors');
   });
 
   it('should load as a raw loader to support images', async () => {
     const testId = './img/index.js';
+    await webpack(testId, mockRelativeWebpackConfig);
     const stats = await webpack(testId, mockBaseWebpackConfig);
 
     const cacheLoaderCallsData = buildCacheLoaderCallsData(

diff --git a/test/compare-option.test.js b/test/compare-option.test.js
@@ -1,11 +1,17 @@
 const fs = require('fs');
+const path = require('path');
 
-const { webpack } = require('./helpers');
+const del = require('del');
+
+const { getRandomTmpDir, webpack } = require('./helpers');
+
+const mockRandomTmpDir = getRandomTmpDir();
 
 const mockCacheLoaderCompareFn = jest.fn();
 const mockWebpackConfig = {
   loader: {
     options: {
+      cacheDirectory: mockRandomTmpDir,
       compare: (stats, dep) => {
         mockCacheLoaderCompareFn(stats, dep);
         return true;
@@ -14,9 +20,27 @@ const mockWebpackConfig = {
   },
 };
 
+const mockCacheLoaderCompareOnRelativeFn = jest.fn();
+const mockRelativeWebpackConfig = {
+  loader: {
+    options: {
+      cacheContext: path.resolve('./'),
+      cacheDirectory: mockRandomTmpDir,
+      compare: (stats, dep) => {
+        mockCacheLoaderCompareOnRelativeFn(stats, dep);
+        return true;
+      },
+    },
+  },
+};
 describe('compare option', () => {
   beforeEach(() => {
     mockCacheLoaderCompareFn.mockClear();
+    mockCacheLoaderCompareOnRelativeFn.mockClear();
+  });
+
+  afterAll(() => {
+    del.sync(mockRandomTmpDir);
   });
 
   it('should call compare function', async () => {
@@ -50,4 +74,21 @@ describe('compare option', () => {
     expect(dep.mtime).toBeDefined();
     expect(dep.path).toBeDefined();
   });
+
+  it('should call compare with contextualized dep', async () => {
+    const testId = './basic/index.js';
+    await webpack(testId, mockWebpackConfig);
+    await webpack(testId, mockRelativeWebpackConfig);
+    mockCacheLoaderCompareFn.mockClear();
+
+    const stats = await webpack(testId, mockRelativeWebpackConfig);
+
+    expect(stats.compilation.warnings).toMatchSnapshot('warnings');
+    expect(stats.compilation.errors).toMatchSnapshot('errors');
+    expect(mockCacheLoaderCompareOnRelativeFn).toHaveBeenCalled();
+
+    // eslint-disable-next-line
+    const dep = mockCacheLoaderCompareOnRelativeFn.mock.calls[0][1];
+    expect(path.isAbsolute(dep.path)).toBeTruthy();
+  });
 });
diff --git a/test/helpers.js b/test/helpers.js
@@ -1,8 +1,10 @@
+const os = require('os');
 const path = require('path');
 
 const del = require('del');
-const webpack = require('webpack');
 const MemoryFS = require('memory-fs');
+const uuidV4 = require('uuid/v4');
+const webpack = require('webpack');
 
 const moduleConfig = (config) => {
   return {
@@ -91,6 +93,11 @@ function compile(fixture, config = {}, options = {}) {
   );
 }
 
+function getRandomTmpDir() {
+  return path.resolve(os.tmpdir(), `test_${uuidV4()}`);
+}
+
 module.exports = {
+  getRandomTmpDir,
   webpack: compile,
 };
diff --git a/test/precision-option.test.js b/test/precision-option.test.js
@@ -1,10 +1,15 @@
-const { webpack } = require('./helpers');
+const del = require('del');
+
+const { getRandomTmpDir, webpack } = require('./helpers');
+
+const mockRandomTmpDir = getRandomTmpDir();
 
 const mockCacheLoaderCompareFn = jest.fn();
 const mockCacheLoaderCompareWithPrecisionFn = jest.fn();
 const mockWebpackConfig = {
   loader: {
     options: {
+      cacheDirectory: mockRandomTmpDir,
       compare: (stats, dep) => {
         mockCacheLoaderCompareFn(stats, dep);
         return true;
@@ -15,6 +20,7 @@ const mockWebpackConfig = {
 const mockWebpackWithPrecisionConfig = {
   loader: {
     options: {
+      cacheDirectory: mockRandomTmpDir,
       compare: (stats, dep) => {
         mockCacheLoaderCompareWithPrecisionFn(stats, dep);
         return true;
@@ -30,6 +36,10 @@ describe('precision option', () => {
     mockCacheLoaderCompareWithPrecisionFn.mockClear();
   });
 
+  afterAll(() => {
+    del.sync(mockRandomTmpDir);
+  });
+
   it('should not apply precision', async () => {
     const testId = './basic/index.js';
     await webpack(testId, mockWebpackConfig);