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 error in TS client packages with '--isolatedModules': #2473

Closed
wants to merge 1 commit into from

Conversation

haggholm
Copy link

error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided.

error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided.
@charmander
Copy link
Collaborator

Assuming this is an error you get when trying to use values of MessageName in your code and not just when you import pg, it looks like you could fix this by specifying a string literal instead?

@haggholm
Copy link
Author

Assuming this is an error you get when trying to use values of MessageName in your code and not just when you import pg…

Unfortunately, that’s not an accurate assumption: our code doesn’t import MessageName at all. I suppose that’s triggered by library type checks with --isolatedModules.

@ujwal-setlur
Copy link

Would love to see this merged. @types/pg is breaking with --isolatedModules set:

DefinitelyTyped/DefinitelyTyped#51183

@bart-opplane
Copy link

#2481

@rudi-c
Copy link

rudi-c commented Mar 2, 2021

+1 we've run into this too

@charmander
Copy link
Collaborator

Is this fixable by using string literals in pg-protocol, then? A non-const enum seems less than ideal here.

@Bartmr
Copy link

Bartmr commented Mar 2, 2021

@charmander why not? It's just type declarations. The final value will always be one of those strings

@brianc
Copy link
Owner

brianc commented Mar 12, 2021

Sorry I missed this for a while, had some personal stuff to deal with. This looks good, and this also looks good...I'll take a look at both today & get one merged & released. I only need const enum in 1 place because that enum gets inlined into the switch statement in the hot path of the parser. Everywhere else I don't need const enum - particularly on exported types. I'm pretty sure I just put the const in that other place out of habit.

@brianc
Copy link
Owner

brianc commented Mar 12, 2021

I just merged this: #2490

This should fix the issue, yeah? LMK if that looks good & I can release a new version w/ the change.

@brianc
Copy link
Owner

brianc commented Mar 12, 2021

hmmm thinking about it do you think the other way is gonna break any backwards compatibility? If those MessageName enums are used somewhere then changing them to a string literal is not gonna work. OTOH if they were never exportable to begin with because they were const enum then there's no risk of anyone having already used them?

@charmander
Copy link
Collaborator

@charmander why not? It's just type declarations. The final value will always be one of those strings

Exactly, it’s redundant.

@forivall
Copy link
Contributor

Considering that the MessageName const enum was never a top level export, I think it can be considered to be implementation internal details, so anyone directly using it would be accepting that breakage risk.

@brianc
Copy link
Owner

brianc commented Mar 18, 2021

Considering that the MessageName const enum was never a top level export, I think it can be considered to be implementation internal details, so anyone directly using it would be accepting that breakage risk.

okay awesome i'll keep it how it is then & get a new version pushed up today/tomorrow...thanks for your help & feedback & sorry for the delay!

@davidje13
Copy link

@brianc did you manage to get a version pushed with this fix? I'm still seeing the problem after rebuilding my node_modules / package-lock.json (so it should be latest versions of everything):

node_modules/pg-protocol/dist/messages.d.ts:86:21 - error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided.

86     readonly name = MessageName.copyData;

@brianc
Copy link
Owner

brianc commented Apr 13, 2021

did you manage to get a version pushed with this fix? I'm still seeing the problem after rebuilding my node_modules / package-lock.json (so it should be latest versions of everything):

sorry - doing that right now!

@brianc
Copy link
Owner

brianc commented Apr 13, 2021

sorry for the delay

Successfully published:
 - pg-connection-string@2.5.0
 - pg-cursor@2.6.0
 - pg-pool@3.3.0
 - pg-protocol@1.5.0
 - pg-query-stream@4.1.0
 - pg@8.6.0

published w/ the fix - finally!

Please lmk if you still have problems & thanks for the input & help here. ❤️

@brianc brianc closed this Apr 13, 2021
@bart-opplane
Copy link

@brianc The problem still persists. @types/pg still has the old versions of pg-protocol and others

@davidje13
Copy link

@bart-opplane I cleared my package-lock.json and it brought in the latest version with the fix. It might be good for @types/pg to force the new version, but because it's a version range it will already pick it up if you don't have an old version locked.

@bart-opplane
Copy link

@davidje13 You're right. But not just package-lock.json

Deleting node_modules, package-lock.json and the file generated by Typescript's incremental compilation all together solves the issue.

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

9 participants