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(types): module exports for TypeScript for ES6 compliance. #918

Merged
merged 4 commits into from
Dec 8, 2023

Conversation

hjdhjd
Copy link
Contributor

@hjdhjd hjdhjd commented Nov 26, 2023

Addresses #878 (comment) and corrects the export declaration in TypeScript.

@robertsLando robertsLando changed the title Fix module exports for TypeScript for ES6 compliance. fix(types): module exports for TypeScript for ES6 compliance. Nov 27, 2023

export declare function createBroker (options?: AedesOptions): Aedes
Copy link
Member

Choose a reason for hiding this comment

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

why did you removed this?

aedes.d.ts Outdated
declare module 'aedes' {
export = Aedes
}
export { default } from './types/instance';
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is correct

@hjdhjd
Copy link
Contributor Author

hjdhjd commented Nov 27, 2023

It is correct, and I've confirmed its use.

To explain it more verbosely:

// Since Aedes is a default export from './types/instance', we can do this:
export { default } from './types/instance';

// If Aedes was NOT the default export, we would need to create an alias for the default export like so:
// export { default as Aedes } from './types/instance';

Agree on createBroker...I can add it back in, though I suspect it's superfluous. Let me know. The bug this fixes prevents me from using the latest Aedes and I'm forced to use 0.49 in the meantime.

@robertsLando
Copy link
Member

Could you add a typescript test that covers this to prevent any possible breaking change?

Add additional validation testing for createBroker.
@hjdhjd
Copy link
Contributor Author

hjdhjd commented Dec 5, 2023

Could you add a typescript test that covers this to prevent any possible breaking change?

Added an additional check for createBroker, but there's not much more I can do here. Do let me know if you're going to accept this or not - I'd like to get this PR closed out either way. Thanks.

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

Please fix lint issues.

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

Lint issues, fix them: npm run lint:fix

@hjdhjd
Copy link
Contributor Author

hjdhjd commented Dec 6, 2023

Linting done.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7117819105

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.739%

Totals Coverage Status
Change from base Build 6798861432: 0.0%
Covered Lines: 816
Relevant Lines: 816

💛 - Coveralls

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@robertsLando robertsLando merged commit 6662b37 into moscajs:main Dec 8, 2023
14 checks passed
@hjdhjd hjdhjd deleted the patch-1 branch December 9, 2023 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants