egg-core icon indicating copy to clipboard operation
egg-core copied to clipboard

add symbol type to loadToApp function property argument

Open windmemory opened this issue 5 years ago • 8 comments

Checklist
  • [x] npm test passes
  • [x] documentation is changed or added
  • [x] commit message follows commit guidelines
Affected core subsystem(s)

loader

Description of change

Add symbol type to the property argument of loadToApp function. The js implementation supports this type of argument, but the typescript declaration does not support this, currently we can not pass symbol variable to be the property argument of loadToApp. As we use typescript heavily, we want to enable this type of argument.

windmemory avatar May 28 '20 07:05 windmemory

Codecov Report

Merging #220 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #220   +/-   ##
=======================================
  Coverage   99.80%   99.80%           
=======================================
  Files          19       19           
  Lines        1016     1016           
=======================================
  Hits         1014     1014           
  Misses          2        2           
Impacted Files Coverage Δ
lib/loader/egg_loader.js 100.00% <ø> (ø)

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 ca04a45...c2fe695. Read the comment docs.

codecov[bot] avatar May 28 '20 07:05 codecov[bot]

when could we load to a symbol?

atian25 avatar Aug 14 '20 05:08 atian25

You can not do this in TS now, because the definition is not allowed, but you can do this in JS. This PR wants to fix the TS version problem.

windmemory avatar Aug 14 '20 05:08 windmemory

my question is why will we call loadToApp to a symbol property?

atian25 avatar Aug 14 '20 05:08 atian25

I would suggest you ask the person who write the code in JS.

If the type of argument is supported by JS, then we should add a TS declaration for it, does this make sense?

windmemory avatar Aug 14 '20 05:08 windmemory

sorry, could you show me the link for the js code that you just mentioned?

atian25 avatar Aug 14 '20 05:08 atian25

Please check:

https://github.com/eggjs/egg-core/blob/77e11f5fb38b3646fb20deb78bbc239b06bf8349/lib/loader/egg_loader.js#L379

And here: https://eggjs.org/zh-cn/advanced/framework.html#%E8%87%AA%E5%AE%9A%E4%B9%89-loader

The property is a key in a js map, and can be string, or Symbol, and you can see the official docs shows using a Symbol to be the key of a property.

windmemory avatar Aug 14 '20 06:08 windmemory

Symbol.for('egg#loader') which is used for the framework, it's not the same thing with loadToApp.

this.app[property] doesn't mean it's suggested to use Symbol, it's just js, we don't have to modify the file to forbid Symbol.

and we don't find the scenario to loadToApp with some Symbol, so I think it should not add to typings.

atian25 avatar Aug 14 '20 06:08 atian25