The Wayback Machine - https://web.archive.org/web/20201216071755/https://github.com/chartjs/Chart.js/pull/7121
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

Enable Typedoc excludeNotExported #7121

Merged
merged 1 commit into from Feb 20, 2020
Merged

Conversation

@benmccann
Copy link
Collaborator

@benmccann benmccann commented Feb 19, 2020

This turns on excludeNotExported in typedoc

It also contains a lot of workarounds for TypeStrong/typedoc#393 to make it work

@benmccann
Copy link
Collaborator Author

@benmccann benmccann commented Feb 19, 2020

Sigh... This works locally, so I'm not sure why the tests are failing on the VMs

@kurkle
Copy link
Collaborator

@kurkle kurkle commented Feb 19, 2020

Probably need the exclude that was removed few days ago

@kurkle
Copy link
Collaborator

@kurkle kurkle commented Feb 19, 2020

Try if that one helps: #7099 (comment)

@kurkle
Copy link
Collaborator

@kurkle kurkle commented Feb 19, 2020

Ok, no other ideas to try.

@benmccann benmccann force-pushed the benmccann:static-fields branch from b7961e5 to 068f151 Feb 19, 2020
@kurkle
Copy link
Collaborator

@kurkle kurkle commented Feb 19, 2020

Oh, I read somewhere that you need to put the export on separate line in some cases (ie, end of file), when exporting a class as default (and using properties)

@benmccann
Copy link
Collaborator Author

@benmccann benmccann commented Feb 19, 2020

Well that would defeat the entire purpose of this PR :-p

@benmccann
Copy link
Collaborator Author

@benmccann benmccann commented Feb 19, 2020

This isn't worth spending more time on to me. I'd rather just not use excludeNotExported

@benmccann benmccann closed this Feb 19, 2020
@kurkle
Copy link
Collaborator

@kurkle kurkle commented Feb 19, 2020

I was supposed to comment here about moving commonjs() lower in the plugins. But it was rollup-plugin-istanbul that is failing here. I think its using babel 6 still.

I think I got it working in #7125 (need to extract the chores from that)

@benmccann benmccann reopened this Feb 19, 2020
@benmccann benmccann force-pushed the benmccann:static-fields branch from 068f151 to 1eea407 Feb 19, 2020
@benmccann
Copy link
Collaborator Author

@benmccann benmccann commented Feb 19, 2020

That worked! Thank you so much!!

@benmccann benmccann force-pushed the benmccann:static-fields branch 2 times, most recently from 8570314 to 503d332 Feb 19, 2020
@benmccann benmccann changed the title Export classes directly Enable Typedoc excludeNotExported Feb 19, 2020
@benmccann benmccann force-pushed the benmccann:static-fields branch 5 times, most recently from 2cfcd62 to bb76fd9 Feb 19, 2020
@benmccann benmccann requested review from etimberg and kurkle Feb 20, 2020
Copy link
Member

@etimberg etimberg left a comment

It is quite particular about those exports :|

@benmccann
Copy link
Collaborator Author

@benmccann benmccann commented Feb 20, 2020

Yeah, hopefully they fix that bug in Typedoc, but I think it's generally better this way anyway

Copy link
Collaborator

@kurkle kurkle left a comment

Not sure about exporting the singleton classes. Maybe we should at least add a comment there that its only exported for typedocs?

babel.config.json Show resolved Hide resolved
karma.conf.js Show resolved Hide resolved
src/core/core.animator.js Show resolved Hide resolved
src/core/core.defaults.js Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
@benmccann
Copy link
Collaborator Author

@benmccann benmccann commented Feb 20, 2020

Ok, I just added some comments, but am just as happy to remove the export if you think that's preferrable

For the Animator, I think that class will mostly just be used internally and probably isn't something users would interact with, so removing the export and removing from the docs would be fine

For the Defaults, there's only one method: set. I just noticed that #7074 removed the _ prefix, so I just went ahead and removed the @private. I don't think the docs show this being used at all though and use alternate ways of setting the default. If you want to consider this class as internal for now as well I'm also happy to remove the export

@benmccann benmccann dismissed stale reviews from etimberg and kurkle via c9ddcab Feb 20, 2020
@benmccann benmccann force-pushed the benmccann:static-fields branch from b299fb0 to c9ddcab Feb 20, 2020
@kurkle
kurkle approved these changes Feb 20, 2020
@etimberg etimberg merged commit e2145e3 into chartjs:master Feb 20, 2020
5 checks passed
5 checks passed
build (ubuntu-latest) build (ubuntu-latest)
Details
build (windows-latest) build (windows-latest)
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.001%) to 93.978%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.