Feat: warn when the package from resolved alias is not available #160

fatfisz · 2017-04-09T19:55:17Z

This is the last feature branch for now. Thanks for bearing with me!

In #144 some users reported that the plugin stopped working for them and the reason was a fix for applying the plugin twice (fixed in #146).

This was not necessarily a problem on the plugin side, but rather bad configuration. Still, an expectation was that the plugin would somehow be working in that case. Here's an example of such configuration:

{
  "presets": ["es2015"],
  "plugins": [
    ["module-resolver", {
        "root": "./src",
        "alias": {
          "basic": "components/1-basic",
          "atoms": "components/1-basic/1-atoms",
          "molecules": "components/1-basic/2-molecules",
          "organisms": "components/2-organisms"
        }
      }]
    ]
}

The repo where this configuration was being used didn't have a package named "components", but rather this was the name of a directory in that repo. The expectation was that "components/1-basic" (bad) would be treated as "./src/components/1-basic" (good).

This change does that - after resolving using the alias, the path is resolved again using root.

I understand that this behavior may be confusing, but this should be alleviated by a "debug" functionality.

This is not a breaking change, because regular expressions are not really released yet.

Breaking change: aliased paths are resolved again using root and cwd.

codecov · 2017-04-09T19:55:19Z

Codecov Report

Merging #160 into beta will not change coverage.
The diff coverage is 100%.

Impacted Files	Coverage Δ
src/normalizeOptions.js	`100% <100%> (ø)`	⬆️
src/getRealPath.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9f9bf6...fda055b. Read the comment docs.

codecov · 2017-04-09T19:55:19Z

Codecov Report

Merging #160 into beta will not change coverage.
The diff coverage is 100%.

Impacted Files	Coverage Δ
src/utils.js	`100% <100%> (ø)`	⬆️
src/normalizeOptions.js	`100% <100%> (ø)`	⬆️
src/getRealPath.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52600af...647ce41. Read the comment docs.

tleunen

Interesting simplification to transform all alias into regexp and then use them for the path resolver.

I think in most cases, it would resolve the issue, but I wonder if some people specify relative alias path in their babel config without using ./. Probably not because it would have been taken as a node module by the plugin... (And thus the issue you found).

I believe it's ok. Not sure yet if it's a good thing to do, or warn that the alias doesn't exist in node_modules or in the project, but it exists in ${root}/${alias}, and then warn the user to update its config.

tleunen · 2017-04-09T20:33:39Z

src/normalizeOptions.js

+      .map((key) => {
+        let value = alias[key];
+
+        if (value.startsWith('npm:') && !warnedAboutNpmPrefix) {


Lets drop this for good :)

If you say so :) I'm just worried about breaking some more configs :D

It will be a 3.0 so no problem removing the deprecated things :) Especially since there's a way to do it without using the prefix.

tleunen · 2017-04-09T20:35:10Z

src/normalizeOptions.js

-        opts.regExps.push([new RegExp(key), substitute]);
+    const regExpAliases = aliasKeys
+      .filter(isRegExp)
+      .map(key => getAliasPair(key, alias[key]));


Would be best to do both case in the same map loop.

Ok, will join those cases.

fatfisz · 2017-04-09T20:58:58Z

So do you think I should roll back the double-resolve and there should be just a warning if the package is missing?

Sorry, maybe it's too late (almost midnight in Poland :D), but I didn't your intent from the second part of the comment.

Breaking change: support for the "npm:" prefix is removed

tleunen · 2017-04-10T16:18:09Z

Hehe.

What I was trying to say is if we should should really resolve with the root config after the alias, or only warn the user the alias config requires relative paths starting with ./ or a node_module dependency.
And if we decide to go for the warning, we could automatically detect that the user did something wrong in his config and suggest something.
Like in your example,
Instead of having "basic": "components/1-basic", we could suggest "basic": "./src/components/1-basic", because we found out the path exists (./src comes from the root config).

I'm not saying to do that. Just thinking out loud here.

Could you explain how the existing code in the PR will work for this config? I'm not sure I fully understand the alias myself.

fatfisz · 2017-04-10T18:55:00Z

Thanks for clearing that up!

In the config you've linked all the aliases point at packages or files that are in the packages, so in this case aliases should behave as they did up till now. I have to say though that this config is very confusing now that I've taken a clear look at it - @expo/vector-icons is pointing react-native-vector-icons and vice versa... but this is another issue :)

The conflict is only possible when after aliasing you end up with a path that is both a filename available in one of the roots and a package name. But! This is also possible for resolving with the root on its own.

At this point however I'm tempted to go with the warning instead. After aliasing a simple require.resolve would do to check if the package exists (ofc only in case the path is not starting with a dot). I don't feel strongly about the current solution, so I could go either way.

So what, should I give it a go?

fatfisz · 2017-04-10T21:19:12Z

Heads up: I'm adjusting the tests and going with the warning.

tleunen · 2017-04-10T21:19:27Z

Yeah, let's try how a warning will work. (Feel free to keep the code from this PR in case we need it later ;))

We can even simplify it at first by warning when normalizing the options, instead of trying to resolve it

fatfisz · 2017-04-10T21:21:04Z

I'm not sure if warning at the option processing time is a good idea, resolve results may vary depending on the file where the import is.

tleunen · 2017-04-10T21:26:23Z

Depends what/why we warn though. If we check that the node_module exists OR the path exists, it should be fine doing it in normalizeOptions, right?

fatfisz · 2017-04-10T21:35:20Z

What if there is node_modules nested in a directory?

package root
|- node_modules/
|- directory/
|  |- node_modules/
|  \- file.js
\- .baberc

In this case in file.js some other packages may be available. That may be a contrived example, but I think it's better to cover all bases here. I think that the overhead will be comparable, if not better: the plugin is initialized separately for each file, so all of the aliases will be processed in normalizeOptions compared to only the necessary ones in the file.

tleunen · 2017-04-10T21:37:33Z

Hmm, your exemple is a bit weird, since it should probably not really happen. At least for this plugin. Even if you can have that kind of structure with sub modules, I doubt you'll have a alias on something on another module ;)

Go for what you have in mind, that's fine :) We'll see how it works

fatfisz · 2017-04-10T21:55:01Z

Ok, let's make life more complicated (sorry, I just love coming up with those absurd scenarios):

mono-repo root
|- package1/
|  |- node_modules/
|  \- file.js
|- package2/
|  |- node_modules/
|  \- file.js
|- package3/
|  |- node_modules/
|  \- file.js
\- .baberc

Let's say that in the subpackages we want to alias package to package/lib using only one .babelrc config, but multiple node_modules. Not including the .babelrc is ok, because the packages are published in the built form anyway. Still contrived, but shows a different angle :)

fatfisz · 2017-04-10T22:59:54Z

@tleunen Finished!

tleunen · 2017-04-12T16:21:42Z

Thanks @fatfisz, I'll check the PR a bit later today. Sorry for the delay again.

tleunen

I think that's pretty cool.
Just a nit comment, otherwise I'm ok with this update.

Do you have any other changes in mind or we're good to deploy a beta version? If so I'll do in the next few days, after testing it quickly in my project at work.

tleunen · 2017-04-13T04:26:43Z

src/normalizeOptions.js

+      }
+    });
+
+    opts.alias = [...nonRegExpAliases, ...regExpAliases];


What do you think of this?

opts.alias = aliasKeys.reduce((acc, key) => { acc.push( isRegExp(key) ? getAliasPair(key, alias[key]) : getAliasPair(`^${key}((?:/|).*)`, `${alias[key]}\\1`) ); return acc; }, []);

I wanted to preserve the order: standard aliases before regexps. At this point I think it could go either way, as the order should be taken from the object.

Note: according to the spec, the order of enumerating the object keys is not guaranteed to be the same as in the literal, so in the future it may be needed to allow an array-type declaration. We're currently relying on V8.

Indeed, one or the other, we'll have an issue if a user expects the order to be kept. With an object, it's not guaranteed as you say. We'll figure out a way later if really needed. For now, it doesn't seem like users need it.

Ok, I'll go with the single array then.

(btw. map is even better than forEach here, will use that instead)

tleunen · 2017-04-13T04:36:35Z

src/getRealPath.js

+  try {
+    resolve.sync(modulePath, {
+      basedir: currentFile,
+      extensions: nodeExtensions,


I missed that, but actually it should be the custom extensions the user sets, or the babel extension.

so extensions from opts.

And we should reuse this function in findPathInRoots to remove the duplicate.

I'm not sure about this one - the packages are 3rd party and so they will probably have standard extensions.

OTOH it shouldn't hurt much to include more files in the lookup, as long as they are package-ish (what should be ensured by require.resolve). In the worst case there will be false positives, which would result in some warnings being silenced, but it doesn't sound very bad.

I'll go with your suggested change.

Hmm.. Maybe I missed a detail, but it was used in the alias config to test the alias was actually pointing to an existing file (or node_modules). By 3rd party, you mean a node_module package, right?

If we really want to be safe, we could use the user/babel extensions for local files/modules and then the node extensions for the ones under node_modules (the ones who don't start with ./). But would it be really useful? Doesn't seem like it's really worth it to me. Again, we'll iterate if really needed, otherwise I'm ok with potential false positives. Anyway, this is only a helper for the user. We never checked the alias was correct in any previous versions so it's already a bit better now :)

Yes, a node_modules package. And yes, let's just see if anyone complains at all :)

fatfisz · 2017-04-13T19:40:16Z

@tleunen After this I'd like to merge #149. This will be a nice addition for the beta version.

tleunen

lgtm, good to merge once you answered my small comment :)

tleunen · 2017-04-14T06:04:23Z

test/index.test.js

            'regexp-priority$': './miss',
-            'regexp-priority': './hit',
+            'regexp-priority': './miss',


Coul you explain why this changed?

The test was invalidated because the order is now from the top to the bottom instead of aliases first, then reg exps :)

Breaking change: aliased paths are resolved again using root and cwd.

Breaking change: The "npm:" prefix has been removed.

fatfisz added 6 commits April 9, 2017 20:16

Join aliases and regular expressions (no change in behavior)

501b839

Always localize aliased paths (also reg exp)

fa481cd

This is not a breaking change, because regular expressions are not really released yet.

Move the aliasedSourceFile processing outside the loop

3e15eec

Print a deprecation warning when npm: is in the config

b42ceb2

Try to resolve aliased paths using the root and cwd

c55a2d0

Breaking change: aliased paths are resolved again using root and cwd.

Add a test for root and alias resolving order

fda055b

tleunen reviewed Apr 9, 2017

View reviewed changes

fatfisz added 2 commits April 9, 2017 23:07

Remove support for npm: completely

024dcb1

Breaking change: support for the "npm:" prefix is removed

Do only one pass over the aliasKeys array

81d528d

fatfisz added 4 commits April 10, 2017 22:10

Fix a typo

da1fcd9

Don't resolve aliased paths again

1b4f87f

Restore the test for the double alias application

c59bcee

Remove a useless test (alias is not resolved if it's a package name)

040bb1c

Make most alias paths into relative ones

7c71d3e

fatfisz force-pushed the resolve-alias-using-root branch from ff27386 to 7c71d3e Compare April 10, 2017 22:33

fatfisz added 3 commits April 11, 2017 00:46

Remove obsolete tests

ce36607

Warn if the package cannot be resolved after aliasing

5b1b900

Fix paths in the tests.

3caaaa1

fatfisz changed the title ~~Feat: resolve alias again using root~~ Feat: warn when the package from resolved alias is not available Apr 11, 2017

tleunen approved these changes Apr 13, 2017

View reviewed changes

tleunen reviewed Apr 13, 2017

View reviewed changes

fatfisz added 3 commits April 13, 2017 12:26

Use opts.extensions for resolving the aliased module path

c0baaa0

Add a missing test for the production environment

66bc9c6

Al aliases should be tried in the enumeration order

dc01d6d

fatfisz added 2 commits April 13, 2017 23:29

Merge branch 'beta' into resolve-alias-using-root

8e55566

Add a useful test

647ce41

tleunen approved these changes Apr 14, 2017

View reviewed changes

tleunen merged commit 1ef8130 into beta Apr 18, 2017

tleunen deleted the resolve-alias-using-root branch April 18, 2017 03:09

tleunen pushed a commit that referenced this pull request Apr 23, 2017

Feat: warn when the package from resolved alias is not available (#160)

9af02ef

Breaking change: aliased paths are resolved again using root and cwd.

tleunen pushed a commit that referenced this pull request Apr 23, 2017

Feat: warn when the package from resolved alias is not available (#160)

c937f79

Breaking change: The "npm:" prefix has been removed.

fatfisz mentioned this pull request Oct 10, 2017

make alias config take precedence over root config #218

Closed

Feat: warn when the package from resolved alias is not available #160

Feat: warn when the package from resolved alias is not available #160

Conversation

fatfisz commented Apr 9, 2017

codecov bot commented Apr 9, 2017

Codecov Report

codecov bot commented Apr 9, 2017 • edited Loading

Codecov Report

tleunen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatfisz commented Apr 9, 2017 • edited Loading

tleunen commented Apr 10, 2017

fatfisz commented Apr 10, 2017

fatfisz commented Apr 10, 2017

tleunen commented Apr 10, 2017

fatfisz commented Apr 10, 2017

tleunen commented Apr 10, 2017

fatfisz commented Apr 10, 2017

tleunen commented Apr 10, 2017

fatfisz commented Apr 10, 2017

fatfisz commented Apr 10, 2017

tleunen commented Apr 12, 2017

tleunen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tleunen Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatfisz commented Apr 13, 2017

tleunen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 9, 2017 •

edited

Loading

fatfisz commented Apr 9, 2017 •

edited

Loading

tleunen Apr 13, 2017 •

edited

Loading