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.

Sign up for GitHub

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

Jump to bottom

feat: allow autoloading of non-default exports #115

Merged
merged 3 commits into from
Jan 12, 2019
Merged

feat: allow autoloading of non-default exports #115

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f71978d
feat: allow autoloading of non-default exports
whefter Dec 22, 2018
1f796f5
feat: add comments to module autoload logic
whefter Dec 23, 2018
16ef3de
feat: expand module loading logic to allow multiple exports from sing…
whefter Jan 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,24 @@ boring. You can automate this by using `loadModules`.
> * `module.exports = ...`
> * `module.exports.default = ...`
> * `export default ...`
>
> To load a non-default export, set the `[RESOLVER]` property on it:
>
> ```js
> const { RESOLVER } = require('awilix');
> export class ServiceClass {
> }
> ServiceClass[RESOLVER] = {}
> ```
>
> Or even more concise using TypeScript:
> ```typescript
> // TypeScript
> import { RESOLVER } from 'awilix'
> export class ServiceClass {
> static [RESOLVER] = {}
> }
> ```

Imagine this app structure:

Expand Down
12 changes: 10 additions & 2 deletions src/__tests__/load-modules.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,17 @@ describe('loadModules', function() {
const container = createContainer()

class SomeClass {}
class SomeNonDefaultClass {
static [RESOLVER] = {}
}

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 },
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@whefter whefter Dec 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤔

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

'nopeClass.js': { SomeClass }
}
const moduleLookupResult = lookupResultFor(modules)
const deps = {
Expand All @@ -31,10 +36,13 @@ describe('loadModules', function() {

const result = loadModules(deps, 'anything')
expect(result).toEqual({ loadedModules: moduleLookupResult })
expect(Object.keys(container.registrations).length).toBe(3)
expect(Object.keys(container.registrations).length).toBe(4)
expect(container.resolve('standard')).toBe(42)
expect(container.resolve('default')).toBe(1337)
expect(container.resolve('someClass')).toBeInstanceOf(SomeClass)
expect(container.resolve('someNonDefaultClass')).toBeInstanceOf(
SomeNonDefaultClass
)
})

it('uses built-in formatter when given a formatName as a string', function() {
Expand Down
38 changes: 26 additions & 12 deletions src/load-modules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,26 +97,40 @@ export function loadModules(
return undefined
}

if (!isFunction(loaded)) {
if (loaded.default && isFunction(loaded.default)) {
// ES6 default export
if (isFunction(loaded)) {
// for module.exports = ...
return {
jeffijoe marked this conversation as resolved.
Show resolved Hide resolved
name: m.name,
path: m.path,
value: loaded,
opts: m.opts
}
}

if (loaded.default && isFunction(loaded.default)) {
// ES6 default export
return {
name: m.name,
path: m.path,
value: loaded.default,
opts: m.opts
}
}

// no export found so far - loop through non-default exports, but require the RESOLVER property set for
// it to be a valid service module export.
for (const key of Object.keys(loaded)) {
if (isFunction(loaded[key]) && RESOLVER in loaded[key]) {
jeffijoe marked this conversation as resolved.
Show resolved Hide resolved
return {
name: m.name,
path: m.path,
value: loaded.default,
value: loaded[key],
opts: m.opts
}
}

return undefined
}

return {
name: m.name,
path: m.path,
value: loaded,
opts: m.opts
}
return undefined
})
result.filter(x => x).forEach(registerDescriptor.bind(null, container, opts))
return {
Expand Down