Skip to content
This repository was archived by the owner on May 10, 2021. It is now read-only.

Fix Windows support #101

Merged
merged 1 commit into from
Nov 24, 2020
Merged

Fix Windows support #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/workflow.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ jobs:
timeout-minutes: 30
strategy:
matrix:
os: [ubuntu-latest, macOS-latest]
os: [ubuntu-latest, macOS-latest, windows-latest]
# We should test on 10.13.0 but don't due to a bug in Jest
# https://github.com/facebook/jest/issues/9453
node-version: [10.15.0, 14.x]
exclude:
- os: macOS-latest
node-version: 10.15.0
- os: windows-latest
node-version: 10.15.0
fail-fast: false
steps:
- name: Git checkout
Expand Down
6 changes: 3 additions & 3 deletions cypress/fixtures/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"name": "next-on-netlify-test",
"scripts": {
"build": "../../../node_modules/.bin/next build",
Copy link
Contributor Author

@ehmicky ehmicky Nov 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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": "next build",
"postbuild": "node ../../../next-on-netlify",
"preview": "../../../node_modules/.bin/netlify dev",
"preview": "netlify dev",
"predeploy": "mkdir -p .git",
"deploy": "../../../node_modules/.bin/netlify deploy --json > deployment.json"
"deploy": "netlify deploy --json > deployment.json"
}
}
5 changes: 3 additions & 2 deletions cypress/plugins/buildProject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
const { join } = require("path");
const { spawnSync } = require("child_process");
const execa = require("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"], {
Copy link
Contributor Author

@ehmicky ehmicky Nov 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/sindresorhus/execa#preferlocal
This removes the need for ../node_modules/.bin/....

});

console.log(" Done! ✅");
Expand Down
9 changes: 5 additions & 4 deletions cypress/plugins/deployProject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const waitOn = require("wait-on");
const { spawn, spawnSync } = require("child_process");
const execa = require("execa");
const { join } = require("path");
const getBaseUrl = require("./getBaseUrl");

Expand All @@ -10,9 +10,10 @@ const deployLocally = ({ project }, config) => {
// Start server. Must start in detached mode, so that we can kill it later.
// Otherwise, we seem unable to kill it.
// See: https://medium.com/@almenon214/killing-processes-with-node-772ffdd19aad
const server = spawn("npm", ["run", "preview"], {
const server = execa("npm", ["run", "preview"], {
cwd: join(config.buildsFolder, project),
detached: true,
localDir: true,
});

// Set deployment
Expand All @@ -36,9 +37,9 @@ const deployOnNetlify = ({ project }, config) => {
process.stdout.write(`Deploying project: ${project}...`);

// Trigger deploy
const deploy = spawnSync("npm", ["run", "deploy"], {
const deploy = execa.sync("npm", ["run", "deploy"], {
cwd: join(config.buildsFolder, project),
encoding: "utf-8",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed.
https://github.com/sindresorhus/execa/blob/2706b3f878345d99e890ff3261d9a683ebcd7dfe/index.js#L41

localDir: true,
});

// Verify success
Expand Down
3 changes: 1 addition & 2 deletions lib/helpers/getDataRouteForRoute.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

};

module.exports = getDataRouteForRoute;
137 changes: 123 additions & 14 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
},
"devDependencies": {
"cypress": "^5.1.0",
"execa": "^4.1.0",
"folder-hash": "^3.3.3",
"husky": "^4.3.0",
"jest": "^26.4.2",
Expand Down
6 changes: 4 additions & 2 deletions tests/customRedirects.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Test next-on-netlify when a custom distDir is set in next.config.js

const { EOL } = require("os");
const { parse, join } = require("path");
const { readFileSync } = require("fs-extra");
const buildNextApp = require("./helpers/buildNextApp");
Expand Down Expand Up @@ -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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes the need for toString().

);

const redirects = contents.toString().trim().split(/\n/);
const redirects = contents.trim().split(EOL);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows, this is \r\n.

expect(redirects[0]).toEqual("# Custom Redirect Rules");
expect(redirects[1]).toEqual(
"https://old.example.com/* https://new.example.com/:splat 301!"
Expand Down
24 changes: 14 additions & 10 deletions tests/defaults.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Test default next-on-netlify configuration

const { parse, join } = require("path");
const { parse, join, sep } = require("path");
const {
existsSync,
readdirSync,
Expand Down Expand Up @@ -44,28 +44,32 @@ describe("next-on-netlify", () => {
describe("next-on-netlify", () => {
test("builds successfully", () => {
expect(buildOutput).toMatch("Next on Netlify");
expect(buildOutput).toMatch("Copying public/ folder to out_publish/");
expect(buildOutput).toMatch("Copying static NextJS assets to out_publish/");
expect(buildOutput).toMatch(
"Setting up API endpoints as Netlify Functions in out_functions/"
`Copying public${sep} folder to out_publish${sep}`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File paths are normalized with join() in the source code, so those messages print backslashes on Windows.

);
expect(buildOutput).toMatch(
"Setting up pages with getInitialProps as Netlify Functions in out_functions/"
`Copying static NextJS assets to out_publish${sep}`
);
expect(buildOutput).toMatch(
"Setting up pages with getServerSideProps as Netlify Functions in out_functions/"
`Setting up API endpoints as Netlify Functions in out_functions${sep}`
);
expect(buildOutput).toMatch(
"Copying pre-rendered pages with getStaticProps and JSON data to out_publish/"
`Setting up pages with getInitialProps as Netlify Functions in out_functions${sep}`
);
expect(buildOutput).toMatch(
"Setting up pages with getStaticProps and fallback: true as Netlify Functions in out_functions/"
`Setting up pages with getServerSideProps as Netlify Functions in out_functions${sep}`
);
expect(buildOutput).toMatch(
"Setting up pages with getStaticProps and revalidation interval as Netlify Functions in out_functions/"
`Copying pre-rendered pages with getStaticProps and JSON data to out_publish${sep}`
);
expect(buildOutput).toMatch(
"Copying pre-rendered pages without props to out_publish/"
`Setting up pages with getStaticProps and fallback: true as Netlify Functions in out_functions${sep}`
);
expect(buildOutput).toMatch(
`Setting up pages with getStaticProps and revalidation interval as Netlify Functions in out_functions${sep}`
);
expect(buildOutput).toMatch(
`Copying pre-rendered pages without props to out_publish${sep}`
);
expect(buildOutput).toMatch("Setting up redirects");
expect(buildOutput).toMatch("Success! All done!");
Expand Down
2 changes: 1 addition & 1 deletion tests/fixtures/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "next-on-netlify-test",
"scripts": {
"next-build": "../../../../node_modules/.bin/next build",
"next-build": "next build",
"next-on-netlify": "node ../../../next-on-netlify"
}
}
4 changes: 2 additions & 2 deletions tests/helpers/buildNextApp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class NextAppBuilder {
// cache the result
if (!existsSync(this.__cachePath)) {
// Build the nextJS app
npmRun("next-build", this.__stagingPath);
await npmRun("next-build", this.__stagingPath);

// Cache the build
copySync(this.__stagingPath, this.__cachePath);
Expand All @@ -84,7 +84,7 @@ class NextAppBuilder {
copySync(this.__cachePath, this.__appPath);

// Run next-on-netlify
const { stdout } = npmRun("next-on-netlify", this.__appPath);
const { stdout } = await npmRun("next-on-netlify", this.__appPath);
return stdout;
}

Expand Down
Loading