feat: allow autoloading of non-default exports #115

whefter · 2018-12-22T17:32:36Z

Module default exports are discouraged in e.g. tslint; the no-default-export setting is set to true when using out-of-the-box settings for the quite popular gts styles.

This proposed PR allows for a non-default export to be loaded if it contains a static property with the RESOLVER key, even if that property is just an empty object. The dropped default export requirement would allow for better compliance with these tslint best practices.

Because the default export is tried first and non-default exports must have the RESOLVER key, the change should be fully backward-compatible.

Tests have been amended to cover this case. Suggestions are welcome!

coveralls · 2018-12-22T17:36:08Z

Coverage remained the same at 100.0% when pulling 16ef3de on whefter:feature/autoload-non-default-export into dc67d7a on jeffijoe:master.

jeffijoe · 2018-12-22T17:37:57Z

I'm also trying to not use default exports, but I personally prefer the default export over having to import a decorator or—in this case—a Symbol.

But as you've noticed, the RESOLVER symbol exists because not everybody shares this opinion, so I see no issue in allowing this.

I'll take a look. 😄

src/load-modules.ts

jeffijoe · 2018-12-22T17:43:19Z

src/__tests__/load-modules.test.ts


    const modules: any = {
      'nope.js': undefined,
      'standard.js': jest.fn(() => 42),
      'default.js': { default: jest.fn(() => 1337) },
-      'someClass.js': SomeClass
+      'someClass.js': SomeClass,
+      'someNonDefaultClass.js': { SomeNonDefaultClass },


What should happen if there are multiple class exports in a file? Which one wins?

I can see two possibilities:

the first one wins (current logic)

an error is thrown

I thought about that while writing the code and figured letting the first one win would be the simplest. Now that you got me thinking about it some more, I'd say throwing an error is the safest course, simply because it won't create hard-to-debug cases where someone's service is not being loaded without any notice as to why.

Of course, we'll have to loop through all the exports instead of just looping until we find a valid one, but files are only loaded once in the vast majority of all scenarios.

Also, it might seem weird if you have a default export and also one marked with the symbol, which would win? Not saying anyone would have a reason to do this, but its an interesting question.

In that case, the default one wins because it's the first one being checked. Again, we could check for all cases, and throw an error.

It might seem like an odd limitation. But named exports always have a name, so perhaps we should use the name of the class/function instead of the file name for non-defaults, but you then risk registering way more than you bargained for, which was another reason I was against doing this.

Thoughts?

Good point. In general, I'd say promoting accepted good practice should be the general guide here, e.g. one class/major function per file. In this case, limiting to one service per file.

Although, as I said above, I don't think it matters too much how much you allow as long as errors are thrown fast and hard. Determine the name like that:

default export -> use file name

named export having [RESOLVER] property

use name from inline resolver options

otherwise use constructor name

otherwise use file name

At every step, check if a service with that name already exists, and, if so, throw. As far as I can see, that should leave no room for unexpected overwriting.

Registering more than you bargained for should be hard if the [RESOLVER] property really is required for an export to be loaded, since it would have to be explicitly set on the export. Under the above logic, name collisions would not go unnoticed.

Right, I forgot about needing the symbol.

On one hand I am reluctant about this because I’m trying to avoid API bloat, but I also don’t want to be too opinionated. 🤔

So I think the logic would be

default -> file name passed through formatName

named with [RESOLVER] symbol prop -> the name passed through formatName

Then we don't have to throw any error either. I think that makes sense? That way this is definitely not going to be a breaking change.

To be honest I was never really a fan of allowing customization using the RESOLVER symbol, I never do it personally, but it was requested in #34 and OP wasn't the first to request it. I really dislike having such a large API (many ways of doing things) but I also don't want to prevent people from being productive either.

Sorry for the delay, am now back from Christmas vacation.

Yes, I understand. It's certainly not vital functionality, it's just irritating when one follows best practices from Google (gts) and then gets red tslint errors on a killer third party library.

If it helps your decision, I was inspired by this: https://github.com/Vincit/objection.js/blob/84b91aead05d834f3e6df9bbead2734090641520/lib/utils/resolveModel.js#L56

For what it's worth, I think "API bloat" would begin once you started e.g. accepting multiple services per file. Then you have to extend the logic to check they all have an explicit name, which means more checks, etc.

Then you have to extend the logic to check they all have an explicit name, which means more checks, etc.

Well, I think it would actually be simpler to just register every named export that defines the RESOLVER symbol, that way we don't have to throw an error if we just use the export name rather than the file name for those (file name would be for default export only)

jeffijoe

So for some reason GH Mobile does not submit reviews. I wrote this in December.

jeffijoe · 2018-12-26T13:51:53Z

src/__tests__/load-modules.test.ts


    const modules: any = {
      'nope.js': undefined,
      'standard.js': jest.fn(() => 42),
      'default.js': { default: jest.fn(() => 1337) },
-      'someClass.js': SomeClass
+      'someClass.js': SomeClass,
+      'someNonDefaultClass.js': { SomeNonDefaultClass },


So I think the logic would be

default -> file name passed through formatName

named with [RESOLVER] symbol prop -> the name passed through formatName

Then we don't have to throw any error either. I think that makes sense? That way this is definitely not going to be a breaking change.

To be honest I was never really a fan of allowing customization using the RESOLVER symbol, I never do it personally, but it was requested in #34 and OP wasn't the first to request it. I really dislike having such a large API (many ways of doing things) but I also don't want to prevent people from being productive either.

jeffijoe · 2019-01-07T06:43:47Z

Hey @whefter , did you see my latest comments? 😄

whefter · 2019-01-07T06:55:23Z

Hi, saw them, was unfortunately delayed in my answering. I'll be re-working the commit this week to include every export with a RESOLVER symbol.

Personally I use it all the time - it makes more explicit how the service is handled (singleton/scoped, classic mode injection, ...). Especially where the injection mode is concerned, it makes more sense to me to set that option right there in the file in which I'm writing the constructor.

jeffijoe · 2019-01-07T06:56:57Z

No worries!

Yeah that's fair enough. 😄

Do you agree that using the export name for named exports and file name for default export is the way to go?

whefter · 2019-01-07T07:02:14Z

Yeah. Both could be overwritten by specifying the name option in the RESOLVER symbol, meaning we need to check for duplicates and at least throw an error in that case. Am I missing something?

jeffijoe · 2019-01-07T07:03:22Z

The current behavior for duplicates is simply overwriting, so I think being consistent there is the best approach?

…le file

whefter · 2019-01-12T12:29:07Z

That should be it. I split the one main test into multiple ones, each testing one aspect. As a bonus, the test is now back in its original form, so that should assure backwards compatibility.

Let me know what you think.

jeffijoe

This looks great, excellent tests as well!

jeffijoe · 2019-01-12T13:46:59Z

Perfect, if you could squash this down to 1 commit then I'll merge! 😄

jeffijoe · 2019-01-12T15:39:46Z

Actually nevermind, I'll do a squash + merge here. 😄

jeffijoe · 2019-01-12T15:57:13Z

Shipped in 4.1.0, thanks @whefter !

whefter · 2019-01-13T12:15:46Z

Thanks for merging, glad to contribute! Yes, of course I forgot to squash. There's always something...

feat: allow autoloading of non-default exports

f71978d

jeffijoe requested changes Dec 22, 2018

View reviewed changes

feat: add comments to module autoload logic

1f796f5

jeffijoe reviewed Jan 3, 2019

View reviewed changes

feat: expand module loading logic to allow multiple exports from sing…

16ef3de

…le file

jeffijoe approved these changes Jan 12, 2019

View reviewed changes

jeffijoe merged commit a677ee7 into jeffijoe:master Jan 12, 2019

whefter mentioned this pull request Jan 16, 2019

feat: allow autoloading of named exports jeffijoe/awilix-router-core#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow autoloading of non-default exports #115

feat: allow autoloading of non-default exports #115

whefter commented Dec 22, 2018 •

edited

Loading

coveralls commented Dec 22, 2018 •

edited

Loading

jeffijoe commented Dec 22, 2018

jeffijoe Dec 22, 2018

whefter Dec 23, 2018 •

edited

Loading

jeffijoe Dec 23, 2018

whefter Dec 23, 2018

jeffijoe Dec 23, 2018

whefter Dec 23, 2018

jeffijoe Dec 23, 2018

jeffijoe Dec 26, 2018

whefter Jan 3, 2019

jeffijoe Jan 3, 2019

jeffijoe left a comment

jeffijoe Dec 26, 2018

jeffijoe commented Jan 7, 2019

whefter commented Jan 7, 2019

jeffijoe commented Jan 7, 2019

whefter commented Jan 7, 2019

jeffijoe commented Jan 7, 2019

whefter commented Jan 12, 2019

jeffijoe left a comment

jeffijoe commented Jan 12, 2019

jeffijoe commented Jan 12, 2019

jeffijoe commented Jan 12, 2019

whefter commented Jan 13, 2019

feat: allow autoloading of non-default exports #115

feat: allow autoloading of non-default exports #115

Conversation

whefter commented Dec 22, 2018 • edited Loading

coveralls commented Dec 22, 2018 • edited Loading

jeffijoe commented Dec 22, 2018

Choose a reason for hiding this comment

whefter Dec 23, 2018 • 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

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

jeffijoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffijoe commented Jan 7, 2019

whefter commented Jan 7, 2019

jeffijoe commented Jan 7, 2019

whefter commented Jan 7, 2019

jeffijoe commented Jan 7, 2019

whefter commented Jan 12, 2019

jeffijoe left a comment

Choose a reason for hiding this comment

jeffijoe commented Jan 12, 2019

jeffijoe commented Jan 12, 2019

jeffijoe commented Jan 12, 2019

whefter commented Jan 13, 2019

whefter commented Dec 22, 2018 •

edited

Loading

coveralls commented Dec 22, 2018 •

edited

Loading

whefter Dec 23, 2018 •

edited

Loading