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.

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

@Option decorators not working for custom plugins #1165

Closed
1 task done
socsieng opened this issue Jan 13, 2020 · 9 comments
Closed
1 task done

@Option decorators not working for custom plugins #1165

socsieng opened this issue Jan 13, 2020 · 9 comments
Labels
bug Functionality does not match expectation

Comments

@socsieng
Copy link
Contributor

Expected Behavior

Plugins using @Option decorator should have their options registered like they used to in version 0.15.8.

Actual Behavior

@Options aren't registered.

Steps to reproduce the bug

# uses typescript 0.16.1
git clone https://github.com/socsieng/typedoc-options-example.git
cd typedoc-options-example
npm install
npm run example

# unexpected - outputs: has declaration false

# uses typescript 0.15.8
git checkout v15
npm install
npm run example

# expected outputs: has declaration true

Environment

  • Typedoc version: 0.16.0
  • Node.js version: 8.16.0
  • OS: macOS Catalina 10.15.1 (19B88)
@socsieng socsieng added the bug Functionality does not match expectation label Jan 13, 2020
@socsieng socsieng changed the title @Option decorators not working for plugins @Option decorators not working for custom plugins Jan 13, 2020
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jan 13, 2020

I guess this counts as a request to use @Option externally. Should be fairly simple to do, just need to modify the function that adds the declarations to the Options class to check if the declaration is already there, and call that function again after loading plugins.

@socsieng
Copy link
Contributor Author

+1 for making it available externally. When I was authoring my plugin, I referred to the built-in plugins as examples.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jan 13, 2020

Fixed in 0.16.2 :)

@socsieng
Copy link
Contributor Author

@Gerrit0, this isn't working as aspected (or at least as thought it would)

@Option decorator overload works well, but the option isn't being registered.

The output for the following command is still incorrect:

npm install
npm run example

Expected from branch v15 (has declaration true):

> [email protected] example /**redacted**/typedoc-options-example
> npm run build && typedoc --plugin $(pwd) --version


> [email protected] build /**redacted**/typedoc-options-example
> tsc --project .

** plugin example **
has declaration true
Loaded plugin /**redacted**/typedoc-options-example

TypeDoc 0.15.8
Using TypeScript 3.7.4 from /**redacted**/typedoc-options-example/node_modules/typescript/lib

Actual (has declaration false):

> [email protected] example /**redacted**/typedoc-options-example
> npm run build && typedoc --plugin $(pwd) --version


> [email protected] build /**redacted**/typedoc-options-example
> tsc --project .

** plugin example **
has declaration false
Loaded plugin /**redacted**/typedoc-options-example

TypeDoc 0.16.2
Using TypeScript 3.7.4 from /**redacted**/typedoc-options-example/node_modules/typescript/lib

My current work-around is to call the following from within the plugin:

addDecoratedOptions(app.options);

I think I expected the decorated option for each plugin to be added as the component is loaded instead of after all the plugins have been loaded. That way, the plugin can make use of the options after the component is added.

@Gerrit0 Gerrit0 reopened this Jan 13, 2020
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jan 13, 2020

Hmm that's a bit trickier.

The decorator no longer requires being attached to a component. It just requires a reference to the options (or an application) when run... this was a preemptive step as a part of a long term effort to remove the @Component decorator as it makes individual parts of the program impossible to test.

Declaring options when the component is loaded poses another problem, what happens if/when the component is removed? This is how it worked before the refactor, and if any option declared under CliApplication was used when not running from the Cli it would break with an unhelpful error.

I guess makes sense to expose addDecoratedOptions so that it can be called by users wanting to run it sooner, but I don't really want to encourage continued use of @Component, which the proposed solution does.

@socsieng
Copy link
Contributor Author

Declaring options when the component is loaded poses another problem, what happens if/when the component is removed? This is how it worked before the refactor, and if any option declared under CliApplication was used when not running from the Cli it would break with an unhelpful error.

Would it be feasible to scope options to a component? I notice that there's already the concept of scoping with getDeclarationsByScope which is currently limited to TypeDoc and TypeScript. Could the @Option decorator scope their options to componentName by default? When removing a component, you could then remove all options scoped to the component.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jan 14, 2020

@Option will not do anything with componentName. The @Component decorator is (slowly due to time constraints) going away. New code should not use it. In 0.16 it was removed from Options and Serializers. In 0.17 I hope to remove it from another part of the program (probably not converters yet, but that would be a huge step forward to making TypeDoc usable in parts and not just as a whole).

Even @Option will probably also go away eventually (almost certainly not this year). While it is convenient, it requires mutating a global state, which is really bad for keeping TypeDoc maintainable. Alternatively, maybe it should be renamed to @BindOption(name: string) which does not declare options, and only creates a getter that relies on the instance's state... I probably should have done this in the first place.

@socsieng
Copy link
Contributor Author

Alternatively, maybe it should be renamed to @BindOption(name: string) which does not declare options, and only creates a getter that relies on the instance's state... I probably should have done this in the first place.

This sounds ideal.

What do you want me to do with this issue? I'm currently use the work-around I mentioned previously so I'm not really waiting for this to be resolved.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jan 14, 2020

Let's just use this issue to track the rename to @BindOption, should be a relatively simple change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Functionality does not match expectation
Projects
None yet
Development

No branches or pull requests

2 participants