krakenjs / generator-swaggerize

Yeoman generator for design-driven apis with swagger 2.0 and krakenjs/swaggerize tools.
Other
70 stars 34 forks source link

fix for path bug on Windows (issue #40) #56

Closed giowe closed 8 years ago

giowe commented 8 years ago

this fix will solve the problem reported in issue #40 that affects all Windows users, simply replacing all '\' chars with '/' char in all template-generated paths

subeeshcbabu-zz commented 8 years ago

Is this happening on latest node versions - 4+ as well?

giowe commented 8 years ago

yep, the problem is present in all node versions simply cause the "/" was programmatically generated from the template engine

MikaTas commented 8 years ago

On Windows

yo swaggerize ... npm WARN EJSONPARSE Failed to parse json npm WARN EJSONPARSE Unexpected token 'a' at 35:104 npm WARN EJSONPARSE ze --only=handlers,models,tests --framework express --apiPath config\api.json" ...

from generated package.json There's a invalid escape character \ used in --apiPath config\api.json .. "scripts": { "test": "tape tests/.js", "cover": "istanbul cover tape -- test/.js", "lint": "jshint -c .jshintrc --exclude ./node_modules .", "regenerate": "yo swaggerize --only=handlers,models,tests --framework express --apiPath config\api.json" },

shattar commented 8 years ago

How would using path.sep solve the issue? As I understand it, the problem here is that we cannot put the windows path.sep into the template package.json and other js files, they need to be converted to '/' prior to being applied to the templates, or converted in the templates themselves.

> path.relative('.', 'C:/hi/you');
'..\\..\\..\\hi\\you'

> path.relative('.', 'C:' + path.sep + 'hi' + path.sep + 'you');
'..\\..\\..\\hi\\you'

> path.relative('.', 'C:/hi/you').replace(/\\/g, '/');
'../../../hi/you'

path = require('path').posix might work nicely, but care must be taken to fix up the places where windows paths are still to be encountered, like process.cwd(), __dirname, and other places they may sneak in.

Any opinions on whether .replace(/\\/g, '/') or path = require('path').posix should be the right way to go? Any other options?

lselden commented 8 years ago

A pretty clean alternative: avoid including separators completely by using path.join and path.resolve to add them for you (since they allow an arbitrary number of arguments):

in your example above: path.relative('.', 'C:/hi/you'); becomes path.relative('.', path.join('C:', 'hi', 'you'));

or examples in the code:

fileName = path.join(self.appRoot, 'tests/test' + opath.replace(/\//g, '_') + '.js'); becomes fileName = path.join(self.appRoot, 'tests', 'test' + opath.replace(/\//g, '_') + '.js');

var relativeApiPath = this.apiConfigPath = path.relative(this.appRoot, path.join(this.appRoot, 'config/' + path.basename(this.apiPath))); becomes var relativeApiPath = this.apiConfigPath = path.relative(this.appRoot, path.join(this.appRoot, 'config', path.basename(this.apiPath)));

self.template('_model.js', path.join(self.appRoot, 'models/' + fileName), model); becomes self.template('_model.js', path.join(self.appRoot, 'models', fileName), model);

shattar commented 8 years ago

I think that the point is being completely missed by some of the comments.

We do not want to windows path separator to end up in the generated app, independent of which platform you generated the app from.

The generated app should work fine with posix style path separators on any platform because the path module for Windows seems to handle forward slashes just fine, but the path module for posix doesn't handle backslashes.

Also, getting the generator to properly escape backslashes, such that they are properly escaped in the generated code, is extremely confusing.

On my fork I have replaced the backslashes with forward slashes at the point where I need them to be forward slashes... in the templates. Everywhere else, the current platform path implementation works just fine, even if forward slashes are used on Windows.

I did not do it the way this pull request did it because it changes to forward slashes in intermediate locations in the code and expects downstream manipulations of the paths to not mess it up again. I did it at the very end, where it is needed.

subeeshcbabu-zz commented 8 years ago

Like the idea of converting the windows \\ paths to / (works in all platforms) at the template file. Just before writing the file content, replacing the windows path style to posix style that works everywhere, seems like the best solution.

subeeshcbabu-zz commented 8 years ago

Upcoming 3.0.0 release is going to fix generator issues on windows platform. RC version is already available, for trying out this release.

Details: https://github.com/krakenjs/generator-swaggerize/pull/91 https://github.com/krakenjs/generator-swaggerize/milestone/1

subeeshcbabu-zz commented 8 years ago

Closing this in favor of https://github.com/krakenjs/generator-swaggerize/pull/91