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.

Sign up for GitHub

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

Jump to bottom

Windows support #1

Open
cjblomqvist opened this issue Dec 12, 2014 · 4 comments
Open

Windows support #1

cjblomqvist opened this issue Dec 12, 2014 · 4 comments

Comments

@cjblomqvist
Copy link

The separator is hard-coded as '/' but in reality, node also supports windows which has '' as default separator (see http://nodejs.org/api/path.html#path_path_sep ). This should be automatically detected and all fns that split things up depending upon the separator should should the os-proper separator.
@cjblomqvist
Copy link
Author

I've been digging some more into this and come to the following conclusion:

  • It's easy to add windows support. It's just a matter of getting the full path.js from the joyent/node -repo (dunno if it works in all browsers then but it seemed to work for me anyway)
  • However, browserify's process dependency does not include the process.platform var, because it's just a plain js-file not processed.
  • Thus, it doesn't work anyway because path.js wouldn't recognize the platform

Is it possible to get the process.platform to get set using some kind of preprocessing on the process lib used? Any help is greatly appreciated! If I get a few pointers I'll happily make PRs where appropriate to make everything work for both platforms (nix/windows).

@cjblomqvist cjblomqvist mentioned this issue Dec 12, 2014
Open
@cjblomqvist
Copy link
Author

See the reference for a workaround.

@JamesMessinger JamesMessinger mentioned this issue Aug 17, 2015
Closed
@cjblomqvist cjblomqvist mentioned this issue Nov 12, 2015
Closed
@Nantris
Copy link

Nantris commented Jan 15, 2022 •
edited
Loading

Windows support would be an enormous help for users trying to maintain cross-platform code. This is literally issue #1 - it would be so good to see support for this.

@sync-by-unito sync-by-unito bot mentioned this issue Jun 2, 2022
Closed
@oshi97 oshi97 mentioned this issue Jun 27, 2023
Merged
oshi97 added a commit to yext/studio that referenced this issue Jun 27, 2023
@oshi97
Update createPage to support windows paths (#243
f3ae998 
Paths like `C:\Users\username\Documents\Github\studio-prototype\packages\studio\tests\__mocks__/test.tsx` are not considered absolute by `path.isAbsolute` in the frontend. This is because we are using `path-browserify` (a browser version of nodejs's `path`) which does not have proper windows support. In real nodejs, `path.isAbsolute` will return true for a windows path like the above.
browserify/path-browserify#1

We could look for an alternative but I'm not sure if that will be necessary. I think I'll have a better picture of that when I'm further in with getting the unit tests compatibility.

For the createPage action, the path.isAbsolute check is unnecessary since we do a `filepath.startsWith(pagesPath)` check as well, and `pagesPath` is always absolute. We may want to support a relative pagesPath at which point we may need to add additional validation, but it would probably make more sense for that validation to go into the backend.

J=SLAP-2791
TEST=auto

tests for createPage.ts and AddPageButton are now passing
@chopin2256
Copy link

Oh whoops, just posted this: #31

The module is unusable for Windows users until this is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make it work with windows, but also with posix BBWeb/path-browserify
3 participants
@cjblomqvist @chopin2256 @Nantris