Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix paths when built on windows #5795

Merged
merged 8 commits into from
Dec 10, 2018
Merged

Fix paths when built on windows #5795

merged 8 commits into from
Dec 10, 2018

Conversation

oBusk
Copy link
Contributor

@oBusk oBusk commented Dec 3, 2018

This PR Fixes #4920

So the problem is that when a next.js application is built on windows, the pages-manifest.json file is created with backslashes. If this built application is deployed to a linux hosting enviroment, the server will fail when trying to load the modules.

Error: Cannot find module '/user_code/next/server/bundles\pages\index.js

My simple solution is to modify the pages-manifest.json to always use linux separator (/), then also
modify server/require.js to, when requiring page, replace any separator (\ or /) with current platform-specific file separator (require('path').sep).

The fix in server/require.js would be sufficient, but my opinion is that having some cross-platform consistency is nice.

This change was tested by bulding an application in windows and running it in linux and windows, aswell as building an application in linux and running it in linux and windows. The related tests was also run.

server/require.js Outdated Show resolved Hide resolved
@timneutkens
Copy link
Member

timneutkens commented Dec 9, 2018

I wonder if we can have an integration test for this behavior to make sure we don't break this in the future. I guess the only reliable way would be reading the manifest and checking a path, since it's only used on the server side. You can add this to test/integration/production, which already does a next build before running the tests 👍 Thank you very much 🙏

@oBusk
Copy link
Contributor Author

oBusk commented Dec 10, 2018

Yea as far as I can think that should cover it, seeing how we skip the changes to require.js.

I added (4eb4b10) a simple test for that! Please have a look if it looks good 😄

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

Let's ship it 💯 Awesome PR @oBusk

@timneutkens timneutkens merged commit c867b0c into vercel:master Dec 10, 2018
@oBusk oBusk deleted the windows-slashes branch December 10, 2018 11:54
@timneutkens
Copy link
Member

I just realized this PR was created on master instead of canary

@oBusk oBusk restored the windows-slashes branch December 10, 2018 12:52
timneutkens pushed a commit that referenced this pull request Dec 10, 2018
This PR Fixes #4920

So the problem is that when a next.js application is built on windows, the `pages-manifest.json` file is created with backslashes. If this built application is deployed to a linux hosting enviroment, the server will fail when trying to load the modules.

```
Error: Cannot find module '/user_code/next/server/bundles\pages\index.js
```

My simple solution is to modify the `pages-manifest.json` to always use linux separator (`/`), then also
modify `server/require.js` to, when requiring page, replace any separator (`\` or `/`) with current platform-specific file separator (`require('path').sep`).

The fix in `server/require.js` would be sufficient, but my opinion is that having some cross-platform consistency is nice.

This change was tested by bulding an application in windows and running it in linux and windows, aswell as building an application in linux and running it in linux and windows. The related tests was also run.
# Conflicts:
#	test/integration/production/test/index.test.js
@oBusk
Copy link
Contributor Author

oBusk commented Dec 10, 2018

@timneutkens

I just realized this PR was created on master instead of canary

Hmm I think I branched from a release to have only my changes, and then when creating the PR into canary was getting more changes than I wanted, so I figured it should go into master. I should be able to branch from a release when making changes, right?

Also, was this particular PR solved or should I create a new?

@timneutkens
Copy link
Member

I just cherrypicked it onto canary: 27c0b19

🙌

Basically to keep in mind is always creating a branch from canary since it's in almost all cases a lot in front of master.

@oBusk oBusk deleted the windows-slashes branch December 11, 2018 01:20
@lock lock bot locked as resolved and limited conversation to collaborators Dec 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants