Adds support for aliases that have multiple targets

[itsjohannawren]

Yesterday I had need of aliases having multiple targets. It's somewhat complicated, but generally involves pkg and needing the ability to override certain files that are archived in the produced binary. I came looking to see if support for multiple targets had been added, and seeing that it had not I went to the PRs. I saw #82 and decided to take on the challenge myself.

So what we have here is the code and tests to support aliases with multiple targets. I cheesed it somewhat by turning every alias into one with "multiple" targets (an array) regardless of whether there is one target or twenty. This reduced the overall amount of logic required later. It made sense to me that when an alias is added with multiple targets that they should be searched in the order added; if a target is added after the initial target(s) it would be processed last. That last part I'm open to having it unshift() vs push()... maybe it would be worth adding an options parameter to addAlias() and addAliases(). Definitely open to feedback on that.

I'll admit that I'm not great at writing tests, but I did what I could. If I missed a case you'd like to see just let me know.

I didn't touch the readme since I've found people to be rather possessive of them, but I'm happy to add the documentation to either "Usage" or "Advance usage". Your choice :-)

Signed-off-by: Jeff Walter [email protected]

[itsjohannawren]

It looks like Travis CI is broken.

[Kehrlann]

Hey @JeffWalter , thanks for contributing ! I'll make some time this week to take a look at your PR.

Yep the travis result reporting seems to have failed ... However the CI did run and seems to have found issues on node < 12, if you want to take a look at it: https://travis-ci.org/github/ilearnio/module-alias/builds/715421802

[itsjohannawren]

Looks like the same two tests failed for everything under NodeJS 12. I tried to avoid using anything super new, but looks like I failed. Oddly, I'm not able to reproduce the failures locally under NodeJS 11.15.0, but they pop right up for NodeJS 10.22.0. If the NodeJS versions in the CI are to be trusted, and I'm inclined to believe them based on the nvm install 11.0 line, then something was "fixed" in 15 minor version bumps. I'll take a look tomorrow (read "later today").

[itsjohannawren]

Because I'm terrible at sleep I took a quick look and it seems to be the way I wrote the tests. Well, copy-pasta-edited them. In the case of expect(require('@src/foo')).to.equal('Hello from foo'), it still has to go through the first target function, which expects @src/baz. I think the most logical solution would be to split the tests so they're only expect()ing a single thing, meaning the other functions can be stubs.

[Kehrlann]

Thanks for the contribution ! Here's a first look at the PR, see comments. I like the "standardize by using an array", with either 1 or n elements inside.

Definitely update the README, that sounds like a good idea.

For the tests, I'm definitely in favor of adding new tests for this specific use case rather than editing the existing ones.

I think we should also add tests for non trivial paths in the imports:

• Exercise the second entry in an array of aliases[1]
• Maybe something with a failing import ?

[1]: For example, if your replace the for loop you added (the one with v), and replace that with var aliasTarget = aliasTargets[0] the tests still pass, and I'd expect at least one test to fail.

I'll let it simmer for a while and re-review in a bit.

[Kehrlann]

What do you think of using

Suggested change

	if ((typeof targets === 'object') && (targets.constructor.name === 'Array')) {
	if (targets instanceof Array) {

[Kehrlann]

Just curious, why v ?

[Kehrlann]

Given tghe addAlias method, this should never happen.
I'd remove the else branch, unless we can write a failing test for it.

[Kehrlann]

Remove the noop branch (see comment about the else branch).

[ilearnio]

Hey Jeff. Thanks so much for contributing!

Is there any reason why custom handler functions can't be used?

From README.md:

// Custom handler function (starting from v2.1)
moduleAlias.addAlias('@src', (fromPath, request, alias) => {
  // fromPath - Full path of the file from which `require` was called
  // request - The path (first argument) that was passed into `require`
  // alias - The same alias that was passed as first argument to `addAlias` (`@src` in this case)

  // Return any custom target path for the `@src` alias depending on arguments
  if (fromPath.startsWith(__dirname + '/others')) return __dirname + '/others'
  return __dirname + '/src'
})

It seems to me that you could use fromPath to detect from where the require was called from and eventually you can return whatever target you need for the alias.