ilearnio / module-alias

Register aliases of directories and custom module paths in Node
MIT License
1.76k stars 69 forks source link

Adds support for aliases that have multiple targets #104

Open itsjohannawren opened 4 years ago

itsjohannawren commented 4 years ago

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 https://github.com/ilearnio/module-alias/pull/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 jeff@404ster.com

itsjohannawren commented 4 years ago

It looks like Travis CI is broken.

Kehrlann commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

ilearnio commented 4 years ago

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.