medikoo / es5-ext

ECMAScript extensions (with respect to upcoming ECMAScript features)
ISC License
174 stars 85 forks source link

Rename the folder something else than # #30

Open vjeux opened 9 years ago

vjeux commented 9 years ago

Hey,

The fact that the folder had '#' in its name caused issues with mercurial as by default it considers it to be a temporary file. I was wondering if you would mind renaming it something else as while this is totally valid, it's going to subtly break many places.

Thanks!

medikoo commented 9 years ago

@vjeux I've checked mercurial (on default configuration) and it sees folder without issues:

$ hg --version
Mercurial Distributed SCM (version 3.4)
(see http://mercurial.selenic.com for more information)

Copyright (C) 2005-2015 Matt Mackall and others
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ mkdir foo
$ cd foo
$ mkdir '#'
$ cd '#'
$ touch bar
$ cd ../
$ hg init
$ hg status
? #/bar
$ hg add
adding #/bar

Still, I agree that some may rely on .hgignore (or .gitignore) which will hide those folders from them.

I'm not sure what better alternative to # would be. I'd like to keep it short and self explanatory, in older version it was prototype, but I found it too verbose, and I like it much better as #. Any suggestions?

vjeux commented 9 years ago

You are right, we had an ignore rule for emacs autosave files that start with #. Sorry my bug report was not accurate.

In any case, while it's fun to use esoteric characters, in practice it is prone to cause issues like this. This is especially annoying since this is a dependency of a dependency of a dependency and not even the main purpose.

I think that as your project gets more popular, this is likely going to cause more issues and would be nice to use traditional characters for filenames to avoid very annoying issues like this.

medikoo commented 9 years ago

@vjeux one alternative I see, is to rename x/# into x#, so it'll be placed in root folder. There still be # in name, but dirname wouldn't start with that, so it shouldn't be picked by ignore rules.

DavidSouther commented 8 years ago

Hey! I just found your project in the most unenjoyable way - when a build script totally broke down in the bowels of some other library! Please please please please do not use esoteric characters in file paths - while every filesystem can handle it, many tools can't.

medikoo commented 8 years ago

@DavidSouther Have you reported a bug to tools you used, that can't handle '#' in filename? it's certainly a serious bug, that should be reported :)

DavidSouther commented 8 years ago

There are a multiplicity of reasons tools might not handle esoteric paths, and yes, they should be improved to handle those cases. You have the ability to make it easier for other people to use your package; if you choose not to, that is your decision.

medikoo commented 8 years ago

@DavidSouther it's long time it's that way. I never had problems with it, and it's just two users that reported this issue to me so far.

Additionally, I think it's really bad, that you come here with bug report, asking for an improvement, instead of reporting an obvious bug to issue tracker of tool that's broken. Sorry, but that's totally not convincing to me.

DavidSouther commented 8 years ago

Por que no los dos? (Why not both?)

I unfortunately can't show you the bug report, as it's with an internal tool. I see you are not interested in this change; if the bug report were marked "Closed" I'd have immediately recognized that and not bothered you.

medikoo commented 8 years ago

@DavidSouther what would be changed is is that e.g. array/# would be renamed to array#, and its what this issue is open for. I'm open for suggestions on how to name prototype, so it's short and self explanatory, array/prototype is a no go (it was actually that way in some older version, but it was too verbose in practice).

silenceisgolden commented 8 years ago

I believe I just ran into this using Google App Engine's Managed VMs, since Google Cloud Storage does not allow # as the first character in a directory name. Javascript uses __proto__, so shortened to proto and that might work? Or maybe just p or even .p or possibly @p. Stack overflow related to GAE issue is http://stackoverflow.com/questions/33858024/app-engine-deploy-fails-managed-vm-nodejs

EDIT: seems like GAE is having issues with something other than the # at start of folder name, so debugging a bit at https://github.com/silenceisgolden/es5-ext

medikoo commented 8 years ago

@silenceisgolden At this point plan is to merge constructor name with #, so folder name would be e.g. array#. In past it was array/prototype. I'm not sure about making it array/proto now.

Concerning Google App Engine, questions is what utility is responsible for CJS modules resolution, as it's were problem occurs. Does Google App Engine does that on its own?

silenceisgolden commented 8 years ago

@medikoo I think there is just a name validation check run on the files before they are uploaded to Google Cloud Storage, which from the there the code is run in the App Engine containers. See comment, it seems like they might be treating this as a bug so I'll see what their response is before continuing any patching http://stackoverflow.com/questions/33858024/app-engine-deploy-fails-managed-vm-nodejs?noredirect=1#comment55490045_33858024

medikoo commented 8 years ago

@silenceisgolden great thanks for clarifying that!

silenceisgolden commented 8 years ago

@medikoo according to http://stackoverflow.com/questions/33858024/app-engine-deploy-fails-managed-vm-nodejs?noredirect=1#comment55524984_33858024, GAE should allow for any name, so I'll pull out of this issue.

For those who still feel strongly about this or find it later, the list of @medikoo libs that have deep references into es5-ext are (at least):

That does not count anything outside of @medikoo libs and in userland already, although hopefully everyone just does require('es5-ext') in userland, but maybe not. The argument could be made to have an object API for each library and still require the whole lib, then .property down from there, so internal lib filenames could change in the future without breaking things. The other argument is to bite a bullet and change to a more common lettering scheme. At the very least, my recommendation would be that if a change were to occur, to do a major version bump so not everything in userland breaks immediately, assuming other libs use ^M.m.p in package.json.

vjeux commented 8 years ago

It's included in eslint which is fairly popular :)

eslint@1.3.1 > escope@3.2.0 > es6-map@0.1.1 > es5-ext@0.10.7

silenceisgolden commented 8 years ago

@vjeux To clarify, yes I'm sure it is many more libraries, but the worry is due to the library design of deep linking to portions of the library such as require('es5-ext/array/#/concat'). The folder names can change and not break anything for those who just do require('es5-ext') but for those who deeplink, they will break if they do not update their code before version bumping in package.json

vjeux commented 8 years ago

Makes sense. That's a tough one :(

medikoo commented 8 years ago

Proposal for now: use trailing _ instead of nested #. So e.g. array_ instead of array/#

DavidSouther commented 8 years ago

@medikoo using ..._ in lieu of .../# sounds like a very good compromise, from my reading of this thread.

markwhitfeld commented 3 years ago

Unfortunately the # character is invalid as part of a URI path because it is interpreted as the start of the URL fragment. This causes issues with any cdn that serves the files for npm packages. It causes the path to stop at the folder before the # character. For example: https://cdn.jsdelivr.net/npm/es5-ext@0.10.53/array/#/concat/index.js https://unpkg.com/browse/es5-ext@0.10.53/array/#/concat/index.js and many more... This choice is, unfortunately, restricting the tools available with which this package can be used.

There is a workaround, which is to basically replace the # with %23 in the url before fetching. i.e: https://cdn.jsdelivr.net/npm/es5-ext@0.10.53/array/%23/concat/index.js https://unpkg.com/browse/es5-ext@0.10.53/array/%23/concat/index.js

I have just implemented this fix for StackBlitz in order to resolve https://github.com/stackblitz/core/issues/1390

medikoo commented 3 years ago

@markwhitfeld this package is now rebranded as ext which doesn't use # in paths anymore.

However ext still misses some parts of es5-ext and many of depends have not yet been migrated.

markwhitfeld commented 3 years ago

Oh awesome. Thanks for your reply! The https://github.com/medikoo/memoizee package still uses this, and this is how I got here. I dropped the comment to mention the workaround too. Hopefully it helps someone, and that this es5 stuff becomes less and less needed as the web progresses 😉