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

Path resolution in codesandboxer-fs does not work on windows #33

Open
conradoramalho opened this issue Oct 4, 2018 · 13 comments
Open

Comments

@conradoramalho
Copy link

conradoramalho commented Oct 4, 2018

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' }
@lukebatchelor
Copy link
Collaborator

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.

@conradoramalho
Copy link
Author

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

@Noviny Noviny changed the title Could not create Open in CodeSandbox Path resolution in codesandboxer-fs does not work on windows Oct 4, 2018
@Noviny
Copy link
Collaborator

Noviny commented Oct 4, 2018

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
Copy link
Collaborator

Noviny commented Oct 5, 2018

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.

@adeelibr
Copy link

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

@Noviny
Copy link
Collaborator

Noviny commented Feb 10, 2019

@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.

@adeelibr
Copy link

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

@Noviny
Copy link
Collaborator

Noviny commented Feb 11, 2019

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.

@CaitlinWeb
Copy link

CaitlinWeb commented Apr 12, 2019

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.

@Noviny
Copy link
Collaborator

Noviny commented Apr 15, 2019

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
Copy link
Collaborator

Noviny commented Apr 15, 2019

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

[email protected]

@CaitlinWeb
Copy link

@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.

@nathsimpson
Copy link

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]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants