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

Export Readers for TypeScript #365

Merged
merged 1 commit into from
May 31, 2021
Merged

Conversation

benk-lastyard
Copy link
Contributor

Hi,

The main reason behind this pull request is that I would like to extend one of the barcode readers to create a custom barcode reader of my own. The problem I had is that quagga.d.ts doesn't export the reader class that I need to extend.

This pull request adds all of the reader classes to quagga.d.ts. In doing so I have also made these other changes

  • Added the protected access modifier to most functions in the reader classes that have an underscore prefix. This seemed to be the original intent however without the access modifier they would all be public. Since these classes were not exported previously there should be no usages outside of this project so the change seems safe to me.
  • Added typescript return type to functions where they were missing.
  • Renamed BarcodeReader._decode to BarcodeReader.decode. This is because the EANReader attempts to call that function for each of the supplementary readers so the decode function cannot be protected.

@rollingversions
Copy link

rollingversions bot commented May 28, 2021

Change Log for @ericblade/quagga2 (1.3.0 → 1.4.0)

New Features

  • Export reader classes.

Edit changelog

@benk-lastyard benk-lastyard force-pushed the master branch 2 times, most recently from 9f8e696 to 1a89e5d Compare May 31, 2021 04:23
@ericblade
Copy link
Owner

hi @benk-lastyard thanks for the pull request! I'll take a quick look at it

@benk-lastyard
Copy link
Contributor Author

Awesome!

@ericblade
Copy link
Owner

@benk-lastyard i really do appreciate the typescript updates! When I went through and converted the readers all over to typescript i tried to do a lot of cleanups like that, but obviously i missed some things.

Let me ask you this -- what would you think of adding an index.ts to src/reader, which imports the individual readers, and then exports them, like the first part of the change 1a89e5d#diff-eea04ce1ae4f8822f870f8d3ac9534c16295a179248987b68f7f58eef30d739b

then we only need a single import added to quagga.js, like

import * as Readers from './reader/index';

and then follow that up with

export { 
   ....,
    Readers,
    ....,
}

so if you're extending the reader from external code, you would do something like

import { Readers } from '@ericblade/quagga2';
class MyReader extends Readers.EANReader {
....
}

would that work?

Also, please add yourself to Contributors in package.json, if you'd like credit beyond the github repo!

@ericblade ericblade linked an issue May 31, 2021 that may be closed by this pull request
@ericblade ericblade added this to the 1.4.0 milestone May 31, 2021
@ericblade ericblade self-requested a review May 31, 2021 06:39
@benk-lastyard
Copy link
Contributor Author

@ericblade No problem with your suggestion. I am far from a Typescript master but I'll try and follow your suggested code snippets above and see how far I get.

Should I move BarcodeReader

BarcodeReader,
to the new location as well? What is the risk of breaking any code in the wild?

I'll make a start on the suggested changes in the meantime.

@ericblade
Copy link
Owner

@benk-lastyard Good idea, i was thinking of that, too. I'm not aware of anyone actively using that, so I'm not incredibly worried about it breaking any thing people are actually using, so let's go ahead and move that (and if anyone does complain, I can just reissue it as v2.0.0, which i'll need to do anyway when i remove official node 10 support, which i just made a todo issue for)

TypeScript is mostly great! I've loved using it, but I'm no master at it either!

@benk-lastyard
Copy link
Contributor Author

Hi @ericblade,

Are you able to offer any guidance on how I would update quagga.d.ts in order to export the Readers namespace so that it can be used like so

import { Readers } from '@ericblade/quagga2';
class MyReader extends Readers.EANReader {
....
}

I have tried export interface Readers, export type Readers and export class Readers but as expected none of them seem to be correct because it isn't an interface, type or class!

reader/index.ts looks like so

import BarcodeReader from './barcode_reader';
import TwoOfFiveReader from './2of5_reader';
import NewCodabarReader from './codabar_reader';
import Code128Reader from './code_128_reader';
import Code32Reader from './code_32_reader';
import Code39Reader from './code_39_reader';
import Code39VINReader from './code_39_vin_reader';
import Code93Reader from './code_93_reader';
import EAN2Reader from './ean_2_reader';
import EAN5Reader from './ean_5_reader';
import EAN8Reader from './ean_8_reader';
import EANReader from './ean_reader';
import I2of5Reader from './i2of5_reader';
import UPCEReader from './upc_e_reader';
import UPCReader from './upc_reader';

export {
    BarcodeReader,
    TwoOfFiveReader,
    NewCodabarReader,
    Code128Reader,
    Code32Reader,
    Code39Reader,
    Code39VINReader,
    Code93Reader,
    EAN2Reader,
    EAN5Reader,
    EAN8Reader,
    EANReader,
    I2of5Reader,
    UPCEReader,
    UPCReader,    
}

@benk-lastyard
Copy link
Contributor Author

Hi @ericblade

The code has been updated based on your feedback here

Regards

@ericblade
Copy link
Owner

LGTM :) Thank you!

@ericblade ericblade merged commit 99726f0 into ericblade:master May 31, 2021
@benk-lastyard
Copy link
Contributor Author

Thanks @ericblade

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.

Export reader types
2 participants