Fixing Windows Support #28

weaverryan · 2017-06-16T15:45:33Z

In a few places, we rely (relied) on doing some "string replace" on paths. Obviously, that doesn't play well with the windows \ directory separators. This fixes the few places that rely on this and includes tests that exposed those failures on AppVeyor.

stof · 2017-06-20T12:07:01Z

lib/config/path-util.js

+        // start with outputPath, then join publicPath with it, see if it equals outputPath
+        // in loop, do dirname on outputPath and repeat
+        // eventually, you (may) get to the right path
+        var contentBase = outputPath;


should be let rather than var

weaverryan · 2017-06-20T16:56:44Z

Tests are green 🎆 - this is ready :)

stof · 2017-06-21T10:47:19Z

lib/config/path-util.js

+         * We use the intersection of the publicPath (or manifestKeyPrefix) and outputPath
+         * to determine the "document root" of the web server. For example:
+         *   * outputPath = /var/www/public/build
+         *   * publicPath = /build/


what if publicPath is /subdir/build/ ? AFAIK path.join(contentBase, publicPath) === outputPath will never be true, as outputPath will use \ everywhere, while path.join(contentBase, publicPath) will have some /

You're correct. But, if the user has a publicPath that is not a sub-string of the outputPath, they will already have received an error forcing them to set manifestKeyPrefix (you do not need to set this manually... until you deploy under a subdirectory or set publicPath to a CDN). So, this situation won't happen. I've just pushed a commit to hopefully clarify this - it's a bit complex since different parts of the system are working together. It is possible that the user will set a manifestKeyPrefix that is not a substring of the outputPath, and then they would see this error (because this is not allowed)

stof · 2017-06-21T10:48:43Z

lib/config/path-util.js

+
+        let outputPath = webpackConfig.outputPath;
+        // for comparison purposes, change \ to / on Windows
+        outputPath = outputPath.split('\\').join('/');


is split+join faster than .replace(/\\/g, '/') ?

replace only replaces the first occurrence :/

no, when using a regex, it depends whether you pass a regex matching the first place or all of them (think preg_match vs preg_match_all in PHP). I added the g modifier on my regex.

You're right! I was mixing this up with the non-regex behavior or replace, which only replaces the first. I'll update these to regex (it's less "weird" than this)

I still suggest doing a benchmark (especially before contributing to webpack-manifest-plugin

stof · 2017-06-21T10:50:26Z

lib/webpack/webpack-manifest-plugin.js

+        if (process.platform === 'win32') {
+            moduleAssets[file] = moduleAssets[file].split('\\').join('/');
+        }
+      /* *** MODIFICATION END *** */


Can this be contributed upstream too ?

stof · 2017-06-21T10:53:36Z

test/config/path-util.js

+ *
+ * @returns {boolean}
+ */
+function isWindows() {


this could be a const rather than a function, as the value being returned will always be the same:

const isWindows = process.platform === 'win32' // ... if (isWindows) { this.skip(); }

stof · 2017-06-21T10:54:41Z

test/config/path-util.js

+    });
+
+    describe('generateManifestKeyPrefix', () => {
+        it('manifestKeyPrefix is correctly not required on windows', () => {


shouldn't this be skipped on Unix ?

Ideally, the code for parsing these paths would be really portable and work on any OS (e.g. even on UNIX, it handles the Windows paths correctly). So far, I've only skipped tests when this is not true - i.e. when Windows does not handle Unix correctly or Unix does not handle Windows correctly.

Actually, based on your other suggestion - I've removed all of the skips and just add a bit of isWindows logic in each function to change the paths based on OS. This means all tests will run on both OS's - best of both worlds (and less duplication as you mentioned)

stof · 2017-06-21T10:54:57Z

test/config/path-util.js

+            expect(actualPrefix).to.equal('build/');
+        });
+
+        it('with absolute publicPath, manifestKeyPrefix must be set', () => {


shouldn't this be skipped on windows ?

stof · 2017-06-21T10:55:57Z

test/config/path-util.js

+
+        it('when outputPath and publicPath are incompatible, manifestKeyPrefix must be set', () => {
+            const config = createConfig();
+            config.outputPath = '/tmp/public/build';


We need to have a windows test for that too (or just change the output path here depending on the OS instead of duplicating everything)

Good idea to alternate the path based on OS - doing that!

stof · 2017-06-21T14:18:42Z

test/config/path-util.js

+ * Some tests are very specific to different operating systems.
+ * We use this to only run them when needed.
+ *
+ * @returns {boolean}


@returns is wrong for a constant

stof · 2017-06-21T14:47:01Z

lib/webpack/webpack-manifest-plugin.js

+        // for Windows, we want the keys to use /, not \
+        // (and path.join will obviously use \ in Windows)
+        if (process.platform === 'win32') {
+            moduleAssets[file] = moduleAssets[file].replace(/\\/g).join('/');


this is broken. proper code is .replace(/\\/g, '/')

* Refactoring delicate path logic into path-util * Fixing logic that relied in unix-style paths * Fixed a bug inside webpack-manifest-plugin that used \ for manifest.json key paths on Windows

This PR was squashed before being merged into the master branch (closes #28). Discussion ---------- Fixing Windows Support In a few places, we rely (relied) on doing some "string replace" on paths. Obviously, that doesn't play well with the windows `\` directory separators. This fixes the few places that rely on this and includes tests that exposed those failures on AppVeyor. Commits ------- 78a0680 cs 6164d80 Tweaks thanks to Stof, including combining Unix & windows tests 162d5f5 Moving some validation logic to be more clear about the flow 5f8ff97 Fixing a small bug when reporting the relative output path 6c65840 Fixing Windows support:

weaverryan force-pushed the windows-paths-fix branch 2 times, most recently from 645607c to d84ef11 Compare June 16, 2017 17:38

weaverryan mentioned this pull request Jun 19, 2017

wrong path in manifest.json on windows #33

Closed

stof requested changes Jun 20, 2017

View reviewed changes

weaverryan mentioned this pull request Jun 20, 2017

Forcing babel-loader 7.1 and updating resolve-rc for new argument #41

Merged

weaverryan force-pushed the windows-paths-fix branch from 769faf8 to 01ad7a7 Compare June 20, 2017 16:42

weaverryan changed the title ~~[WIP] Fixing Windows Support~~ Fixing Windows Support Jun 20, 2017

weaverryan force-pushed the windows-paths-fix branch from e6f65fa to dba5d71 Compare June 20, 2017 17:08

stof requested changes Jun 21, 2017

View reviewed changes

stof reviewed Jun 21, 2017

View reviewed changes

stof approved these changes Jun 21, 2017

View reviewed changes

stof requested changes Jun 21, 2017

View reviewed changes

weaverryan added 4 commits June 21, 2017 10:51

Fixing Windows support:

6c65840

* Refactoring delicate path logic into path-util * Fixing logic that relied in unix-style paths * Fixed a bug inside webpack-manifest-plugin that used \ for manifest.json key paths on Windows

Fixing a small bug when reporting the relative output path

5f8ff97

Moving some validation logic to be more clear about the flow

162d5f5

Tweaks thanks to Stof, including combining Unix & windows tests

6164d80

weaverryan force-pushed the windows-paths-fix branch from eebc711 to 6164d80 Compare June 21, 2017 14:51

cs

78a0680

weaverryan merged commit 78a0680 into symfony:master Jun 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing Windows Support #28

Fixing Windows Support #28

weaverryan commented Jun 16, 2017 •

edited

Loading

stof Jun 20, 2017

weaverryan commented Jun 20, 2017

stof Jun 21, 2017

weaverryan Jun 21, 2017

stof Jun 21, 2017 •

edited

Loading

weaverryan Jun 21, 2017

stof Jun 21, 2017

weaverryan Jun 21, 2017

stof Jun 21, 2017

stof Jun 21, 2017

stof Jun 21, 2017

weaverryan Jun 21, 2017

stof Jun 21, 2017

weaverryan Jun 21, 2017

weaverryan Jun 21, 2017

stof Jun 21, 2017

stof Jun 21, 2017

weaverryan Jun 21, 2017

stof Jun 21, 2017

stof Jun 21, 2017

Fixing Windows Support #28

Fixing Windows Support #28

Conversation

weaverryan commented Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

weaverryan commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof Jun 21, 2017 • edited Loading

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

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

weaverryan commented Jun 16, 2017 •

edited

Loading

stof Jun 21, 2017 •

edited

Loading