zazoomauro / node-dependency-injection

The NodeDependencyInjection component allows you to standarize and centralize the way objects are constructed in your application.
https://github.com/zazoomauro/node-dependency-injection/wiki
MIT License
280 stars 34 forks source link

Prioritize named exports over default export #134

Closed vilasmaciel closed 4 years ago

vilasmaciel commented 4 years ago

For definitions that contain the property main, the named export that indicates this main should be prioritized over the default export that the file could contain.

For instance, a file that contains two exports:

export class ClassNamed { }

export default class ClassDefault {}

A definition that uses the ClassTwo:

  two:
    class: ./../MultipleExports
    main: ClassNamed

Will end up getting an instance of ClassDefault as the default export has priority over any named export, even when the main property has been set.

codecov[bot] commented 4 years ago

Codecov Report

Merging #134 into master will increase coverage by 0.01%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   99.54%   99.55%   +0.01%     
==========================================
  Files          37       38       +1     
  Lines         436      453      +17     
==========================================
+ Hits          434      451      +17     
  Misses          2        2
Impacted Files Coverage Δ
lib/ContainerBuilder.js 100% <ø> (ø) :arrow_up:
lib/InstanceManager.js 100% <100%> (ø) :arrow_up:
lib/TagReference.js 100% <100%> (ø)
lib/Loader/FileLoader.js 100% <100%> (ø) :arrow_up:

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 cb5da7b...a8ae0db. Read the comment docs.

rsaladocid commented 4 years ago

@vilasmaciel There is a problem with your solution. If "MultipleExports" exports a class called "MultipleExports" is still prioritized over default export even though the main is not specified.

This "problem" is due to the line 278:

const className = mainClassName || path.basename(classObject)

If there is no main, the file name is used as class name. I think that a better solution is:

fromDirectory = this.container.defaultDir || fromDirectory
const exportedModule = require(path.join(fromDirectory, classObject))
return exportedModule[mainClassName] || exportedModule.default || exportedModule[path.basename(classObject)]

In this case, the solution works fine in every scenario. First, you try to export the class defined by the main. Second, you try to export the default. Finally, you try to export using the file name.

vilasmaciel commented 4 years ago

I've updated the PR with the changes proposed by @rsaladocid, thanks for the comment!

zazoomauro commented 4 years ago

awesome guys! nice job. If there is any change on documentation please make me a PR. Thanks