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

ts: add missing types for conditional exports #1137

Merged

Conversation

dominictwlee
Copy link
Contributor

@dominictwlee dominictwlee commented Dec 12, 2021

This adds type declarations for the conditional workspace and Wallet exports, as detailed in response to the temporary solution in another issue.

I don't think there's a way to conditionally declare them since all this is statically analyzed at build time.

@dominictwlee dominictwlee marked this pull request as draft December 12, 2021 20:31
@dominictwlee dominictwlee marked this pull request as ready for review December 12, 2021 20:37
CHANGELOG.md Outdated Show resolved Hide resolved
@1337mus
Copy link

1337mus commented Dec 17, 2021

@fanatid Would you have some time to review this? This PR will unblock a slew of projects for us. Thanks in advance.

@fanatid
Copy link
Contributor

fanatid commented Dec 17, 2021

I do not know TS good enough for approval that everything ok but LGTM.

@zisko
Copy link

zisko commented Dec 17, 2021

FYI: I patched this onto my local source and it did not fix the TS autocompletion issue.

@zisko
Copy link

zisko commented Dec 17, 2021

I think the problem is more likely in ts/src/utils/common.ts and related to using process.env.BROWSER to determine if we are in a browser.

@fanatid
Copy link
Contributor

fanatid commented Dec 17, 2021

I think the problem is more likely in ts/src/utils/common.ts and related to using process.env.BROWSER to determine if we are in a browser.

Yeah, I also think that problem in isBrowser variable. Not sure that we can use browser field in package.json because it's not related to the building process. Maybe in rollup config? idk

@dominictwlee
Copy link
Contributor Author

dominictwlee commented Dec 17, 2021

FYI: I patched this onto my local source and it did not fix the TS autocompletion issue.

Odd, I did the same and it should work. How did you patch this?

All this should do is build the definitions into the compiled index.d.ts file in the dist folder. It shouldn't have any effect on the outputted js files.

Do you mean getting the workspace namespace to have auto-completion for it's members? I don't think that has ever worked..even in older versions before the isBrowser condition was implemented, because the type definition for workspace is a proxy object casted as any

const workspace = new Proxy({} as any, {})

If we wanted to include strict typing with hints for the actual workspace program names, we'd need to somehow alter/add those definitions at build time.

The Wallet definition should work as expected though.

image

@losman0s
Copy link
Contributor

losman0s commented Dec 17, 2021

I just tried adding these two exports and it seems to work for me (I simply added them to the source in the anchor repo, built, and then applied the corresponding transpiled bit in a hello-world test project).

I don't think isBrowser has anything to do with this. Instead it seems to be the use of raw export.workspace that does not get picked up by the typing system like usual export statements do. To test that you can simply remove the if (!isBrowser) and observe that even then the types for the conditional modules are still missing.

@zisko
Copy link

zisko commented Dec 17, 2021

Yep- I was wrong. I was building into the wrong workspace. It was 3am here :) sorry!

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

6 participants