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.

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

Rename the folder something else than # #30

Open
vjeux opened this issue May 22, 2015 · 22 comments
Open

Rename the folder something else than # #30

vjeux opened this issue May 22, 2015 · 22 comments
Assignees

Comments

@vjeux
Copy link

vjeux commented May 22, 2015

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
Copy link
Owner

medikoo commented May 23, 2015

@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?

@medikoo medikoo self-assigned this May 23, 2015
@vjeux
Copy link
Author

vjeux commented May 25, 2015

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
Copy link
Owner

medikoo commented May 26, 2015

@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
Copy link

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
Copy link
Owner

medikoo commented Oct 12, 2015

@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
Copy link

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
Copy link
Owner

medikoo commented Oct 12, 2015

@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
Copy link

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
Copy link
Owner

medikoo commented Oct 12, 2015

@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
Copy link

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
Copy link
Owner

medikoo commented Nov 23, 2015

@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
Copy link

@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
Copy link
Owner

medikoo commented Nov 23, 2015

@silenceisgolden great thanks for clarifying that!

@silenceisgolden
Copy link

@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
Copy link
Author

vjeux commented Nov 23, 2015

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
Copy link

@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
Copy link
Author

vjeux commented Nov 23, 2015

Makes sense. That's a tough one :(

@medikoo
Copy link
Owner

medikoo commented Feb 16, 2016

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

@DavidSouther
Copy link

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

@markwhitfeld
Copy link

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 stackblitz/core#1390

@medikoo
Copy link
Owner

medikoo commented Jun 7, 2021

@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
Copy link

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 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants