Move PathUtils to Brackets core code?

[ficristo]

I wondered what was PathUtils and found that was coming from
https://github.com/jblas/path-utils
It does not seem used outside Brackets, at least there isn't a package.json or bower.json.
Seems written by and Adobe guy, so, can be relicensed (I think this is a must but not sure) and imported?
My main motivation is very simple: I do not like global variables.

[petetnt]

👍 for this.

Semi-offtopic, but I think that that regexp is amazing / like the comment says, scary:

/^(((([^:\/#\?]+:)?(?:(\/\/)((?:(([^:@\/#\?]+)(?:\:([^:@\/#\?]+))?)@)?(([^:\/#\?\]\[]+|\[[^\/\]@#?]+\])(?:\:([0-9]+))?))?)?)?((\/?(?:[^\/\?#]+\/+)*)([^\?#\.]*(?:\.[^\?#\.]+)*(\.[^\?#\.]+)|[^\?#]*)))?(\?[^#]+)?)(#.*)?/,

[abose]

👍
Pathutils seems to be a submodule. As we are already shipping with the lib, I don't think licensing would be an issue if we are to change the folder where the library is placed in code- as long as the copyright notices are left untampered.

[le717]

We would just need to be sure to continue exposing the global variable with a deprecation warning (like we do with the global CodeMirror) in order to prevent sudden extension breaking.

I am trying to think where PathUtils would be relocated to. I'm thinking either src/file or src/utils. The former already has FIleUtils there, but once the old file system is (eventually) removed, it will be the only module in that folder, but the latter might work better (since it is a utils module).

[zaggino]

Guys, this brings out a question for me ... why do we use git submodules instead of bower/npm modules? For codemirror, mustache, ...

[busykai]

Submodules allow you keep forks of the code and make modifications to them without publishing anywhere. (Take CodeMirror or jslint as an example.) I actually quite disagree that #11734 had to be merged. There's no single technical reason for that: path-utils being submodule has nothing to do with PathUtils being global? If you don't like globals, then the only thing needed to be done was to configure path for requirejs.

Also, commits in #11734 do not specify where it's been taken from, at which version/sha it the snapshot has been taken.

[zaggino]

That's the point @busykai -> do we really need or want to keep forks and make modifications to thirdparty libraries? Take the https://github.com/adobe/CodeMirror2 example, what code do we have in there, which is not present in the upstream https://github.com/codemirror/CodeMirror ? Github says This branch is 53 commits behind codemirror:master so why not just use https://www.npmjs.com/package/codemirror if we don't keep any code specific to Brackets? Also, in extreme cases, you can use npm to point to a github repository ...

[ficristo]

Personally, apart the globals, I do not like submodules too.
And I think using bower (or npm) is better...
Maybe there was a good reason in the past, but now, are they still needed?

[petetnt]

@busykai the original PathUtils explicitly exposes itself to the window so modifications would have been necessary anyway.

I agree that the commits could have had the versions they were taken originally from, but then again the repo has been stale for 4.5 years so I am not sure if it's that relevant (unless the original PathUtils for whatever reason gets completely overhauled or gets an meaningful update at this point which I doubt).

[ficristo]

@busykai @petetnt you are right about the commit, I didn't thought about it...

[zaggino]

I really doubt @jblas is planning to make changes to his 4 years old code anytime soon ...

[busykai]

@zaggino, if you don't have a fork you never have a space to maneuver. If I remember correctly, we needed a modified version of CodeMirror on numerous occasions. jslint is another example. If you're using an npm version (i.e. a publicly distributed version) -- you're stuck. By stuck I mean if you fix it in the dependency upstream, you have to wait for 1) your contribution to be reviewed and accepted; 2) new package to be promoted and published. Logistically, it does not make any sense to wait for any of that. I don't know why would you assume it would never happen, it happened many times now.

I really doubt @jblas is planning to make changes to his 4 years old code anytime soon ...

why not submit a PR to path-utils and clarify the doubt? before taking any action. instead, a 1K PR to brackets is accepted within 2 hours after it is submitted.

@petetnt

the original PathUtils explicitly exposes itself to the window so modifications would have been necessary anyway.

so I could not even figure it was modified. why not contribute the modification upstream? that the open source for you: you contribute to somebody else's code so others could take advantage. if the repository is stale and inactive then you could copy it. in this case i haven't event figured it was modified (because you have nothing to compare it to, no history). the code actually says:

// This file was originally taken from:
// https://github.com/jblas/path-utils/blob/master/path-utils.js

So is it modified or not? Go figure...

@ficristo

Personally, apart the globals, I do not like submodules too.

i really don't think we should be using "I like" as an argument. :) one person's "like" vs another's "don't like" are counterproductive discussions. what make sense and what does not should the ruling force. submodules are enormously useful for our projects, like it or not.

overall, this was worth a discussion before the PR was merged. it should not be done in haste. we should at the very least move the code to thirdparty because it is a third-party code, we have a special place for it. it even has a different license (3-clause BSD).

[petetnt]

@busykai Well yeah, the modification comment was about needing to do more than the only thing needed to be done was to configure path for requirejs. whether it was in the original repo or in Brackets. Of course there is a great chance here of getting stuck in the upstream exactly like you described in your reply to @zaggino, but I agree that contacting @jblas should have been the first step to take (like it was implied in @ficristos first post), which I guess means that the merging might have been too hasty.

I also agree that pulling the code and then modifying the code in separate commits would have made sense to have a cleaner slate for library.

Basically the only part I disagree with is that "I do not like" is a perfectly valid argument against globals because globals are bad, period 🐹

[abose]

Cant we git submodule to thirdparty folder and then just use the library normally instead of using global? like using require(thirdparty/pathutils)?

[petetnt]

@abose like I mentioned above, the file exposes itself as window.PathUtils so it needs to be changed somewhere (like @ficristo did when it ported the file to Brackets repo). Otherwise it's possible as long as someone does the module conversion and it gets merged (I can do the PR tomorrow myself if someone doesn't do it first).

[jblas]

Hey guys, Adobe guy here. :-) The path-utils was written by me on my own time (not Adobe's) hence the license not mentioning Adobe. I also contributed parts of it to jquery-mobile which I was working on at the time. In any case, I tend to write my utils so they are agnostic to the environment they are used in. What exactly are you guys after here? A way to just pull it in via git submodule?

I'm hesitant to add a dependency on requirejs in path-utils.js for the reason I mentioned above, but if it helps you guys out I could checkin a version of path-utils-[something].js that uses requirejs.

[abose]

Or could we contribute to a new branch in the repo, and submodule to that branch from brackets so that the main repo remains tidy?

[jblas]

@abose sure that's fine.

[ficristo]

Hi @jblas. I thought the library was used only for Brackets so I wanted to have in the core instead of a module that seemed unused.
But I think having a bower.json and a package.json would be helpful.
At least I could experiment with Bower in Bracket for my own amusement.

@busykai You are right about personal tastes.
I opened this issue exactly for the discussion, but I don't know how you all manage reviews on Brackets, so I can't comment on the haste or not. And for the license, I am not a lawyer so...
I have also written Maybe there was a good reason in the past, but now, are they still needed? which I can't really understand from your answer.When I read that you do not want to wait PR get merged upstream (JSLint, CodeMirror, ...), I think why are you in haste (sorry couldn't resist) for these PR when in Brackets there are still very old PR?

[busykai]

Or could we contribute to a new branch in the repo, and submodule to that branch from brackets so that the main repo remains tidy?

given @jblas agrees, i believe it's what needs to be done.

[jblas]

@busykai @abose I was also going to mention that the other option is to just fork the path-utils and just have brackets pull in your fork (with requirejs added) as a submodule? Either way I'm fine, though the latter means you can make changes to your hearts delight and not have to rely on me for anything. :-)

[ficristo]

@jblas Will you accept PR to add environment detection? And maybe add something like browserify to automate the tests?

[petetnt]

@ficristo As of jblas/path-utils#1 PathUtils is now a CommonJS/AMD compatible module if you want to redo this 👍

[ficristo]

@petetnt good job!
I prefer to wait #12006 and use package.json directly if possible.