I believe this (along with the original PR#252) fully corrects the original issue #227 as well as the regression #382. #504
Conversation
…iginal issue TypeStrong#227 as well as the regression TypeStrong#382. Also addresses the comments from PR TypeStrong#502, now closed.
src/lib/output/renderer.ts
Outdated
| @@ -201,7 +201,24 @@ export class Renderer extends ChildableComponent<Application, RendererComponent> | |||
| } | |||
| } | |||
|
|
|||
| this.theme = this.addComponent('theme', new DefaultTheme(this, path)); | |||
| let filename = Path.join(path, 'theme.js'); | |||
There was a problem hiding this comment.
Ah, that was the original code that used let (or var), I'll change to const.
src/lib/output/renderer.ts
Outdated
| try { | ||
| const plugin = require(filename); | ||
| if(typeof plugin === 'function') { | ||
| plugin(this.application); |
There was a problem hiding this comment.
Is there a reason we treat default exports and module.exports differently?
There was a problem hiding this comment.
I don't follow what you mean... I'm not terribly familiar with module exports given that there are umd, commonjs, amd and a slew of others.
Can you give me an example of what you mean? I thought I implemented this the way you requested.
There was a problem hiding this comment.
This function is calling plugin(this.application) or new plugin.default(this, path). These should be consistent, it shouldn't change depending on how someone exported it. To make it consistent with wherever else this is done, it'd probably be nice to put this in one place that does something like: function req (id) { return typeof require(id) === 'function' ? require(id) : require(id).default }. This way the behaviour is entirely consistent across the different export styles.
There was a problem hiding this comment.
Trouble is that breaks existing functionality. There was no plugin(this.application) before this PR for theme.js and existing theme.js (that I'm aware of) are sub-classes of DefaultTheme and expect to be passed (Renderer, BasePath). See typedoc-markdown-theme.
The consistency I mentioned was a new alternative for a theme.js file which would be consistent with typedoc plugins but maintain backward compatibility for existing theme.js usage.
There was a problem hiding this comment.
It doesn't break any functionality though, it makes it consistent. What you have is not consistent, the behaviour is different depending on how you are exporting the class/function.
There was a problem hiding this comment.
Ok, what do you want it to be now then?
There was a problem hiding this comment.
For what it's worth, I think there have been very few people that put the theme.js to use and then it wasn't working for a long while anyways. If we're going to add it back in, then it might as well be consistent with the only current paradigm which is typedoc plugins. This would mean:
const ThemeClass = require(filename)(this.application);
this.theme = new (ThemeClass)(path);How does that sound? For those that have existing theme.js "original flavor," it would be a trivial update.
There was a problem hiding this comment.
Just the same code as in #504 (comment)? Check exports.default or use exports directly based on existence of a function. I'm not sure that using the direct result of require is a good idea, it'd make it non-trivial for anyone writing in TypeScript to actually write a plugin, which is why I suggest keeping the .default handling around. I can just change the code myself in the future if that's easier.
|
I was unable to use PascalCase for |
|
Okay, so looks like you changed it to be the way you'd prefer it already then, right? Works for me. |
I believe this (along with the original PR#252) fully corrects the original issue #227 as well as the regression #382.
Also, addresses the comments from PR #502, now closed.