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

fix!: remove serviceClasses #203

Merged
merged 8 commits into from
Apr 8, 2021
Merged

fix!: remove serviceClasses #203

merged 8 commits into from
Apr 8, 2021

Conversation

EndBug
Copy link
Member

@EndBug EndBug commented Apr 6, 2021

Ref #58

After this PR is merged, serviceClasses will no longer be exported, even though it'll still be accessible via Service.getAll(). This is to encourage users to get service classes using Service.get() with aliases, instead of relying on internal class names.

From now on, changes to internal class names should not be marked as breaking changes, since the intended way of getting them is through aliases.

EndBug added 2 commits April 6, 2021 22:27
BREAKING CHANGE: serviceClasses will no longer be an exported member of the package.
@EndBug EndBug added the type: fix Updates to existing functionalities label Apr 6, 2021
@EndBug EndBug requested a review from Snazzah April 6, 2021 20:41
@EndBug EndBug self-assigned this Apr 6, 2021
@codecov
Copy link

codecov bot commented Apr 6, 2021

Codecov Report

Merging #203 (bd950e2) into master (dcf3a73) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head bd950e2 differs from pull request most recent head ccf9e7f. Consider uploading reports for the commit ccf9e7f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #203      +/-   ##
==========================================
+ Coverage   97.80%   97.85%   +0.05%     
==========================================
  Files          46       47       +1     
  Lines        1275     1307      +32     
  Branches      196      196              
==========================================
+ Hits         1247     1279      +32     
  Misses         27       27              
  Partials        1        1              
Impacted Files Coverage Δ
src/Interface/Service.ts 98.36% <ø> (ø)
src/index.ts 100.00% <ø> (ø)
src/Interface/ListIndex.ts 100.00% <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 dcf3a73...ccf9e7f. Read the comment docs.

@Snazzah
Copy link
Member

Snazzah commented Apr 6, 2021

Should there be a way for TypeScript users to get the types for service classes and not necessarily get the class itself from the index? There might be a way to define overrides for Service.get in order for TS to get the right class.

@EndBug
Copy link
Member Author

EndBug commented Apr 7, 2021

Hmmmm, you're right, I didn't think about that...
I'll look into it

@EndBug
Copy link
Member Author

EndBug commented Apr 7, 2021

Ok, so here's what I've changed.
The source code now contains a file that generates a list with every alias and its class, so that it can be used as a type for the Service.get method. That method still uses the same code to actually get the class, but now has a type overload that allows you to get autocompletion on aliases, and the proper typing for the resulting class. If another string is used, it will go back to the old version, which returns a generic null | typeof Service (which is also what appears in the web docs, since using the other signature would result in every service type being listed)

The file is not automatically updated in the build script, to avoid unintentional changes being pushed: the build:source script has to be run manually. To avoid releasing with an outdated list, the tests will fail when that's the case (so don't dismiss the tests!)

What do you think?

@EndBug EndBug requested review from Snazzah and removed request for Snazzah April 7, 2021 14:59
Copy link
Member

@Snazzah Snazzah left a comment

Choose a reason for hiding this comment

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

A bit awkward, but the result works well.

@EndBug EndBug merged commit d8f9e16 into master Apr 8, 2021
@EndBug EndBug deleted the no-serviceClasses branch April 8, 2021 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: fix Updates to existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants