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

Changes the opt-out character from _ to # #151

Merged
merged 3 commits into from
Nov 22, 2016

Conversation

dlmr
Copy link
Member

@dlmr dlmr commented Nov 16, 2016

The change

This change is needed since we might have collisions with internal modules in Node otherwise, as seen here.

An example where this is a problem is in browserify-zlib that has the following code:

var Transform = require('_stream_transform');

Roc will in some instances process this code through its resolver and the following branch will be true.

// request = _stream_transform
if (request.charAt(0) === '_') {
   // stream_transform
   return request.substring(1);
}

This means that the code will now try to require stream_transform instead of the correct _stream_transform. This will then result in an error since this module does not exist.

ModuleNotFoundError: Module not found: Error: Cannot resolve module 'stream_transform' in /project/node_modules/browserify-zlib/src

This change means that opting out now is done in the following way.

-import React from '_react';
-var React = require('_react');
+import React from '#react';
+var React = require('#react');

Question

Is the # a good character to use for opt-out? Is there a better alternative that we can use?

@dlmr dlmr changed the title This changes the opt-out character from _ to # Changes the opt-out character from _ to # Nov 16, 2016
This change is needed since we might have collisions with internal modules in Node otherwise.
@dlmr dlmr force-pushed the fix/change-resolver-opt-out branch from 2cfdcc9 to ab38e69 Compare November 16, 2016 15:13
@lizell
Copy link
Member

lizell commented Nov 16, 2016

Great fix!

I'm not very into what characters to use or not in the require realm, so # works for me.

@andreasrs
Copy link
Member

👍
Looks good.

Like @lizell I do not really have any strong opinions on what character is best for this. I don't know how safe # is. Do you think multiple characters could be considered for this opt-out? It should be rarely used, and the most important thing is that it never collides.

@dlmr
Copy link
Member Author

dlmr commented Nov 16, 2016

When selecting the character for the opt-out we want to make sure it does not match the following.

  1. A normal path.
  2. A npm module.
  3. An internal Node module.

We unfortunately missed that the third point could contain underscore, hence this bug.

So given this conditions we need to use something that is:

  1. Not a path, that is an absolute or relative path in either Windows or Unix. Begins with either ., / or a drive character in Windows like C.
  2. A non valid starting character for a module name, the code used to validate it can be seen here. Basically it means that it should not start with a . or a _ and it needs to be "URL-friendly" (encodeURIComponent should not change the input).
  3. Internal modules might start with _.

This leaves us with some alternatives and the only other requirement that we are after is that it is easy to type and remember. Alternatives include characters like %, &, =, : and >.

@lizell
Copy link
Member

lizell commented Nov 16, 2016

So maybe :? It has a local touch to it if you ask me. ;)

@lizell
Copy link
Member

lizell commented Nov 22, 2016

If the character choice is the only thing holding this PR back, I'm happy with any! ;)

@bjarneo
Copy link
Member

bjarneo commented Nov 22, 2016

I vote for fish. >==#>

If that's a no go, I vote for #.

@dlmr
Copy link
Member Author

dlmr commented Nov 22, 2016

The only problem I have with : is that it might be easy to miss since it does not take as much "space".

require(':react');

require('#react');

With that logic however we do not need to use only a single character but could go for multiple ones like :: or maybe a 🐟 … 😄

@andreasrs andreasrs merged commit 2b9b285 into master Nov 22, 2016
@andreasrs andreasrs deleted the fix/change-resolver-opt-out branch November 22, 2016 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants