Skip to content

Update to latest Typescript version #654

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

Closed
wants to merge 1 commit into from
Closed

Update to latest Typescript version #654

wants to merge 1 commit into from

Conversation

Gerrit0
Copy link
Collaborator

@Gerrit0 Gerrit0 commented Dec 4, 2017

Simple enough - I just fixed the build errors.

Copy link
Collaborator

@aciccarello aciccarello left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. One small question.

@@ -33,24 +33,6 @@ export class ComponentSource extends OptionsComponent {
}
}
}

private removeComponent(component: AbstractComponent<any>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this method removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was breaking the build as it was defined, but never used: from #641:

src/lib/utils/options/sources/component.ts(37,13): error TS6133: 'removeComponent' is declared but its value is never read.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see. I probably should review this class to ensure that there is not an implementation bug here before I merge the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like removeComponent should have been called in onComponentRemoved. I probably should put together a separate PR and do some testing. Looks like the minimal theme triggers removing components.

renderer.removeComponent('assets');
renderer.removeComponent('javascriptIndex');
renderer.removeComponent('navigation');
renderer.removeComponent('toc');

There also looks to be some bugs in removeComponent (i.e. slice on this array will do nothing).

if (index !== -1) {
this.knownComponents.slice(index, 1);
for (let declaration of component.getOptionDeclarations()) {

@RangerMauve
Copy link

What's blocking this PR? Is there anything I could do to help move it forward?

@Gerrit0
Copy link
Collaborator Author

Gerrit0 commented Jan 4, 2018

I believe now that #664 has been merged, this should be good to go. Unfortunately since the tests on the project right now are completely broken (no useful info if something changes) it's dangerous to merge anything.

@RangerMauve
Copy link

That's a shame. For now I'm using your fork directly for my project and it appears to be working fine. I don't have a particularly fancy use-case, though.

@aciccarello
Copy link
Collaborator

There aren't any major blockers right now but I'll want to merge a couple more PRs and do a big dependency update before a release.

@bilby91
Copy link

bilby91 commented Jan 8, 2018

Any update on this?

bilby91 added a commit to suttna/botbuilder-slack that referenced this pull request Jan 11, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Typedoc is not working for our current typescript version.

TypeStrong/typedoc#654
@validData validData mentioned this pull request Jan 19, 2018
aciccarello added a commit that referenced this pull request Feb 1, 2018
classes example modified because accessing abstract properties is not allowed in the constructor

closes #654
closes #655
closes #681
closes #687
@aciccarello
Copy link
Collaborator

@Gerrit0 I didn't merge this PR but I wanted to say thanks for figuring out what needed to be changed ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants