Fix: double backslashes on riot path when doing server-side rendering #2131

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@antoinegoutagny
Contributor

IMPORTANT: for all the pull requests use the dev branch

Answer the following depending on the type of change you want to merge

Code

  1. Have you added test(s) for your patch? If not, why not?
    This introduces changes to lib/server/index.js, not to riot core itself.

  2. Can you provide an example of your patch in use?
    It does not modify existing usage in any way.

Post the link using one of our bug report templates:

  1. Is this a breaking change?
    No

Content

Provide a short description about what you have changed:

Server-side rendering injects a require to riot path in tags (here).

The use of path.resolve() (line 6 of the same file) to get the path to riot uses single backslashes as path separator on Windows, which breaks when trying to interprete the require to riot.

This fix doubles all backslashes in the path returned by path.resolve(), to ensure a usable path on Windows systems. There is no impact on other OS.

@antoinegoutagny antoinegoutagny Fix: double backslashes on riot path when doing server-side rendering
Server-side rendering injects a require to riot path in tags. The use of
path.resolve() to get the path uses single backslashes as path separator
on Windows, which breaks when trying to
require('c:\whatever\the\path\to\riot').

This fix doubles all backslashes in the path returned by path.resolve().
cfb3bb2
@GianlucaGuarini
Member

Thanks but I still don't get the problem and it seems strange this issue was raised only now after 2 years. Are you sure the problem does not occur only on your machine for any other reason? Why don't you simply use the RIOT env variable instead?

$ RIOT=weird\path\to\riot.js node index.js
@antoinegoutagny
Contributor
antoinegoutagny commented Dec 5, 2016 edited

The problem is that path.resolve returns a path with single backslashes. In lib/server/index.js, the variable passed to context._compile (line 29) thus contains something like require('c:\path\to\riot.js') before the tag content itself.
In module.js, the last line of the _compile method is compiledWrapper.apply, where it executes the "virtual" module we gave it in parameter. That's where it throws an error on Windows systems, because single backslashes escape the following character, making the riot path invalid.

The first version where it happens in is 2.3.0, it works fine up to 2.2.4. I don't know why it hasn't been noticed earlier. Maybe few people use server-side rendering, and even fewer do so on Windows? For all that matters, I've reproduced the issue on several coworkers' computers as well.

Sure, using the environment variable works OK as a workaround, but that requires additional, manual steps that could be avoided if that PR were accepted.

The following is a very simple demo of the issue. Unzip it, npm i, node ./app.js and enjoy the error (on WIndows only): riot-pr-2131-demo.zip

@GianlucaGuarini GianlucaGuarini added a commit that referenced this pull request Dec 19, 2016
@GianlucaGuarini GianlucaGuarini closes #2131 2e9a985
@GianlucaGuarini
Member

I finally found enough time to work on this issue @antoinegoutagny my patch solves the issue more elegantly thanks for pointing it out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment