search

geekflyer / gulp-ui5-preload

Create a Component-preload.js file for openui5 / sapui5 projects
33 stars 10 forks source link

Update index.js #1

Closed themasch closed 9 years ago

themasch commented 9 years ago

Added support for windows paths (yeah, there are people who need to use that)

geekflyer commented 9 years ago

Hi @themasch, thanks a lot for your pull request and for reporting this problem with windows.

There's just 2 minor things to be made - then I will merge the pull request: Could you please wrap the whole assignment expression (options.namespace ? .... into a node.js' path.normalize call (https://nodejs.org/api/path.html) instead of using regex for just a part? Also please give an a bit more descriptive commit message why this change was made.

Afterwards I'll merge the pull request right away.

Thanks and Best Regards Christian

themasch commented 9 years ago

Okay, Maybe that fix was a little to quick :)

The Problem with windows is the crazy stupid directory separator (). This leads gulp-ui5-preload to generate paths like 'cool/namspace/view\foo\Bar.view.xml' which does not look nice and is pretty invalid. The correct version would be 'cool/namespace/view/foo/Bar.view.xml'. The path in the JSON referers to the url that maps to the resource, not the the local path on the filesystem, so it always uses '/'.

So what we need is to combine the namespace part with the relative path part but assure that the DS for this part is '/' instead of ('\').

A good way would be to do something like this: path.normalize(namespace + path.relative(base, path)).replace(/\/g, '/').

We could use .split(path.sep).join('/') instead but i'm not really sure if that is much better/faster.

themasch commented 9 years ago

I did some quick benchmarks and it seems like replace is about 20% faster then .split().join().

geekflyer commented 9 years ago

Ok,

I thought path.normalize would convert all kind of paths (including Windows) to Unix style, but it converts them to platform dependant paths. So I think your first change with the regex is fine then. Please just amend the commit message and then I'll merge it.

Best Regards Christian

geekflyer commented 9 years ago

merged.