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: using relative import for provider, fixes problems with intellisense in vscode #712

Merged
merged 2 commits into from Sep 10, 2021

Conversation

Lyr-7D1h
Copy link
Contributor

Fixes vscode intellisense by using a relative import instead of an absolute one.

@@ -28,24 +28,24 @@ export class PactProviderConfigOptionsService
this.animalRepository.clear();
token = '1234';

return 'Animals removed to the db';
return { description: 'Animals removed to the db' };
Copy link
Member

Choose a reason for hiding this comment

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

what is the purpose of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests were failing due to the need of having JsonMap as a return type. Also showing that imports didn't resolve correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't break with the change, though - because nestjs-pact only uses the v2 options type, which hasn't changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really these tests shouldn't be returning anything at all, though - because I don't think it's valid in V2. @mefellows, thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Correct. I actually think it's completely misleading, so would prefer to remove altogether.

@mefellows
Copy link
Member

@TimothyJones is our stance still OK with sticking with relative imports? I recall having a chat but can't see where.

@TimothyJones
Copy link
Contributor

Yep! I think the absolute imports are currently a mistake on my part (although my vscode handles them fine)

@Lyr-7D1h
Copy link
Contributor Author

@TimothyJones for me in the verifier.ts it does resolve correctly but when transpiled and used as a library it does not and assigns an any type.

@TimothyJones
Copy link
Contributor

Thanks for the fix! We'll merge it as soon as we've worked out what's going on with that provider test (I think that test is disabled anyway, because it's not compatible with the next release, see pact-foundation/nestjs-pact#7 ).

As an aside, do you know how to get a typescript module to flatten the types? At the moment, intellisense is happy to deep link into @pact-foundation/pact/some/path, which makes it hard to refactor things without it being a breaking change. I'd like intellisense to still be able to reason about the types, but to get them all from the base import.

@Lyr-7D1h
Copy link
Contributor Author

@TimothyJones Add @internal to the exports you want to hide. add stripInternal: true to tsconfig.json.
image
image
image

It should only use the exports from index.js

From:
https://stackoverflow.com/questions/59122428/typescript-library-hide-internal-exports.

@TimothyJones
Copy link
Contributor

TimothyJones commented Jul 20, 2021 via email

@TimothyJones
Copy link
Contributor

Aha! I finally had a moment to look at this - the problem was indeed a bug that this fix exposed.

export type VerifierOptions = VerifierV3Options & ProxyOptions; 

^ VerifierOptions was clobbering the other VerifierOptions. I'll rename the types accordingly and get this merged in.

@TimothyJones TimothyJones merged commit 278f006 into pact-foundation:feat/v3.0.0 Sep 10, 2021
@TimothyJones
Copy link
Contributor

Thanks again for the PR! Apologies for taking so long on this one. We'll release it this weekend.

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

3 participants