Fix Windows support #101
Fix Windows support #101
Conversation
2b73057 to
7a68fb0
Compare
| @@ -1,10 +1,10 @@ | |||
| { | |||
| "name": "next-on-netlify-test", | |||
| "scripts": { | |||
| "build": "../../../node_modules/.bin/next build", | |||
There was a problem hiding this comment.
This relies on shebangs, which do not work on Windows.
This PR instead relies on execa, which adds shebangs support for Windows.
Note: I co-maintain execa.
|
|
||
| // Build the given NextJS project | ||
| const buildProject = ({ project }, config) => { | ||
| process.stdout.write(`Building project: ${project}...`); | ||
|
|
||
| // Build project | ||
| spawnSync("npm", ["run", "build"], { | ||
| execa.sync("npm", ["run", "build"], { |
There was a problem hiding this comment.
Calling npm with spawn() does not work as intended on Windows without the shell option.
execa handles this case automatically though, providing with Windows support.
| cwd: join(config.buildsFolder, project), | ||
| preferLocal: true, |
There was a problem hiding this comment.
See https://github.com/sindresorhus/execa#preferlocal
This removes the need for ../node_modules/.bin/....
| cwd: join(config.buildsFolder, project), | ||
| encoding: "utf-8", |
There was a problem hiding this comment.
| @@ -10,8 +10,7 @@ const buildId = fileContents.toString(); | |||
| // Return the data route for the given route | |||
| const getDataRouteForRoute = (route) => { | |||
| const filePath = getFilePathForRoute(route, "json"); | |||
|
|
|||
| return join("/_next", "data", buildId, filePath); | |||
| return `/_next/data/${buildId}${filePath}`; | |||
There was a problem hiding this comment.
Paths in the _redirects file must use forward slashes. However, path.join() use backslashes on Windows.
Note that filePath is not prepended with /, otherwise this would create a double slash (which was previously normalized thanks to join().
| @@ -40,10 +41,11 @@ describe("Routing", () => { | |||
| test("includes custom redirect rules", async () => { | |||
| // Read _redirects file | |||
| const contents = readFileSync( | |||
| join(PROJECT_PATH, "out_publish", "_redirects") | |||
| join(PROJECT_PATH, "out_publish", "_redirects"), | |||
| "utf8" | |||
There was a problem hiding this comment.
This removes the need for toString().
| ); | ||
|
|
||
| const redirects = contents.toString().trim().split(/\n/); | ||
| const redirects = contents.trim().split(EOL); |
There was a problem hiding this comment.
On Windows, this is \r\n.
| expect(buildOutput).toMatch( | ||
| "Setting up API endpoints as Netlify Functions in out_functions/" | ||
| `Copying public${sep} folder to out_publish${sep}` |
There was a problem hiding this comment.
File paths are normalized with join() in the source code, so those messages print backslashes on Windows.
| }); | ||
| } catch (error) { | ||
| throw new Error( | ||
| `An error occurred during "npm run ${command}" in ${fromDirectory}: ${error.message}` |
There was a problem hiding this comment.
error.message includes stdout and stderr with Execa.
There was a problem hiding this comment.
Hey @ehmicky, this is amazing!! 🎉🎉🎉 Fantastic work, great to see execa in action, and thanks for all your detailed comments.
I checked it out on my (non-Windows) machine and the tests run very well 👍
PS: On a tangential note: If you have a better idea for how to handle the server-starting and -killing for these Cypress tests, I'd be all ears!
- Support for i18n in Next 10 ([#75](#75)) - dependabot: node-notifier from 8.0.0 to 8.0.1 ([#125](#125)) - Expose Netlify function params as netlifyFunctionParams ([#119](#119)) - Fix: local cypress cache control ([#118](#118)) - Fix: add res.finished to createResponseObject ([#117](#117)) - Fix: Windows support ([#101](#101)) - Improve logs for specified functions/publish dirs ([#100](#100))
Fixes #88.
This adds support for Windows. This fixes tests on Windows, now running them in CI.
Many of Netlify CLI users are on Windows (example: #56 (comment)).