codesandboxer icon indicating copy to clipboard operation
codesandboxer copied to clipboard

Path resolution in codesandboxer-fs does not work on windows

Open conradoramalho opened this issue 7 years ago • 13 comments

I'm using Docz to do documentation and I have this problem: Is it really a bug?

Could not create Open in CodeSandbox { Error: Cannot find module 'D:\Projects\docz\examples\basic\Button' from 'D:\Projects\docz\node_modules\codesandboxer-fs\src'
    at Function.module.exports [as sync] (D:\Projects\docz\node_modules\resolve\lib\sync.js:43:15)
    at loadRelativeFile (D:\Projects\docz\node_modules\codesandboxer-fs\src\loadRelativeFile.js:55:30)
    at Promise.all.internalImports.map.filePath (D:\Projects\docz\node_modules\codesandboxer-fs\src\loadFiles.js:32:14)
    at Array.map (<anonymous>)
    at loadFiles (D:\Projects\docz\node_modules\codesandboxer-fs\src\loadFiles.js:31:21)
    at Object.assembleFiles (D:\Projects\docz\node_modules\codesandboxer-fs\src\assembleFiles.js:85:21)
    at <anonymous> code: 'MODULE_NOT_FOUND' }

conradoramalho avatar Oct 04 '18 00:10 conradoramalho

It would be super hard to tell from this information alone.

Are you able to push up a minimally reproducible test repo that we could look at, with instructions on what to run to reproduce?

It looks like you're using windows, and we would likely be testing on macs, but I dont expect that would be an issue.

lukebatchelor avatar Oct 04 '18 00:10 lukebatchelor

I am using windows

The problem is in the catch of the function: https://github.com/pedronauck/docz/blob/master/packages/docz-utils/src/codesandbox.ts

This are the params that are passed to assembleFiles function:

rawCode:

import { Button } from './Button';

import React from 'react';

const doczStyles = {
  margin: '0 3px',
  padding: '4px 6px',
  fontFamily: '"Source Code Pro", monospace',
  fontSize: 14,
};

const App = ({ children }) => (
  <div style={doczStyles}>
    {children && typeof children === 'function' ? children() : children}
  </div>
)

export default () => (
  <App>
    <Button kind="primary">Click me</Button>
    <Button kind="secondary">Click me</Button>
    <Button kind="cancel">Click me</Button>
    <Button kind="dark">Click me</Button>
    <Button kind="gray">Click me</Button>
  </App>
)

examplePath: src\components\codesandbox.example.csb.js

conradoramalho avatar Oct 04 '18 01:10 conradoramalho

We are using a custom pathResolution function! I have finally accepted the obvious truth that this was a bad idea, so will be looking to replace it in the future.

Currently codesandboxer-fs fundamentally doesn't work on windows.

Noviny avatar Oct 04 '18 13:10 Noviny

Hm. Digging into this a bit, we are likely to need two ways to resolve paths in codesandboxer-fs. One which we use for comparing and writing js paths, and one for system paths to pass to fs to read files.

Probably going to use path-browserify directly to resolve paths that are intended for js use, and path directly for other uses.

This seems sub-optimal, but not sure how else to solve for it.

This work should be done once supporting typescript ships.

Noviny avatar Oct 05 '18 02:10 Noviny

Any update for this? I would love to help for this issue, although I don't know where to start from. I would love to make a PR for this, I would need some guidance 😅 @CompuIves

adeelibr avatar Feb 10 '19 18:02 adeelibr

@adeelibr Hey! Help on this would be greatly appreciated!

There's sort of two parts here. First is just getting good tests around this. I think some of the integration tests should fail on a windows machine, but it would be good to isolate it.

The second one is a bit harder. We do a mix of path.resolve and other joins to connect paths, and some paths are resolved by node, while others use fs-like actions. I did a bit of spelunking into this but didn't come up with a firm way to separate these out, but would be keen to help out if you want to take up solving this.

Noviny avatar Feb 10 '19 23:02 Noviny

The second one is a bit harder. We do a mix of path.resolve and other joins to connect paths, and some paths are resolved by node, while others use fs-like actions. I did a bit of spelunking into this but didn't come up with a firm way to separate these out, but would be keen to help out if you want to take up solving this.

Can you tell me which part of code base is this related to so that I can start digging into the code base. @Noviny

adeelibr avatar Feb 11 '19 07:02 adeelibr

Sure thing.

Hm. Coming back, it might be easier to isolate than I think. I think it's this file, where we are calling fs.readFileSync (several times) using paths that always assume linux paths.

Either that, or it will be the reverse, and places where we are calling path.join to get valid paths to write into .js files are getting the windows separator.

Noviny avatar Feb 11 '19 10:02 Noviny

I encountered this with Docz as well, on Windows 10. After debugging I may have identified the source of the issue. The resolvePath function in utils uses '/' to split and join the file paths.

My use case was a Docz project with relative files in the same directory. I was able to fix the issue for myself by replacing resolvePath() with this:

const path = require('path');

function resolvePath(basePath, relativePath) {
    const { dir } = path.parse(basePath);
    return path.join(dir, relativePath);
}

I doubt this solves for all cases, but maybe @Noviny and @adeelibr can use it as a starting point to fix it. Node's path should be used as much as possible since it already handles cross-platform differences.

CaitlinWeb avatar Apr 12 '19 20:04 CaitlinWeb

Hi @CaitlinWeb! Thanks for looking into this. This might actually just be the fix (and a simple one at that)

Do you have a codesandbox generated by this fixed resolvePath that I could have a look at?

I'm assuming since the path used to read the file is also the path used to write the file location in the sandbox, we'll end up with a bunch of paths like this from windows files:

image

instead of properly nested folders.

This is probably better to have working un-ideal.

I think the best solution is to simply modify the path once when we load files, as in other places we want paths that will make sense in node.

Here's a PR with a proposed solution:

https://github.com/codesandbox/codesandboxer/pull/51/files

If this makes sense and you have time to test it, I'd be delighted, otherwise I'll try and find some time in the next week to ensure this works.

Noviny avatar Apr 15 '19 03:04 Noviny

Published an RC for this in case anyone wants to test:

[email protected]

Noviny avatar Apr 15 '19 03:04 Noviny

@Noviny I don't really have a demo of my fix. I could possibly put one together or at least test the PR when I have a chance. Hopefully within the next week or so.

CaitlinWeb avatar Apr 16 '19 16:04 CaitlinWeb

This has bit me while trying to run docz:build in a BitBucket pipeline (I can't tell if it's Windows or Linux :/ ).

Could not create Open in CodeSandbox Error: Cannot find module '/opt/atlassian/pipelines/agent/build/components/text' from '/opt/atlassian/pipelines/agent/build/node_modules/codesandboxer-fs/src'

I'm using [email protected]

nathsimpson avatar Apr 07 '20 05:04 nathsimpson