Skip to content

ts: add missing types for conditional exports - #1137

Merged
paul-schaaf merged 5 commits into
otter-sec:masterfrom
dominictwlee:ts/add-missing-export-types
Jan 3, 2022
Merged

ts: add missing types for conditional exports#1137
paul-schaaf merged 5 commits into
otter-sec:masterfrom
dominictwlee:ts/add-missing-export-types

Conversation

@dominictwlee

@dominictwlee dominictwlee commented Dec 12, 2021

Copy link
Copy Markdown
Contributor

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
Comment thread CHANGELOG.md Outdated
@dominictwlee
dominictwlee requested a review from fanatid December 13, 2021 14:50
@1337mus

1337mus commented Dec 17, 2021

Copy link
Copy Markdown

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

@fanatid

fanatid commented Dec 17, 2021

Copy link
Copy Markdown
Contributor

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

@zisko

zisko commented Dec 17, 2021

Copy link
Copy Markdown

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

@zisko

zisko commented Dec 17, 2021

Copy link
Copy Markdown

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

fanatid commented Dec 17, 2021

Copy link
Copy Markdown
Contributor

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

dominictwlee commented Dec 17, 2021

Copy link
Copy Markdown
Contributor Author

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

losman0s commented Dec 17, 2021

Copy link
Copy Markdown
Contributor

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

zisko commented Dec 17, 2021

Copy link
Copy Markdown

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

@paul-schaaf
paul-schaaf merged commit 6f4fe1d into otter-sec:master Jan 3, 2022
akash-osec pushed a commit to akash-osec/anchor that referenced this pull request Mar 25, 2026
@jamie-osec jamie-osec added this to the Pre-1.0 milestone Jun 24, 2026
akash-osec pushed a commit to akash-osec/anchor that referenced this pull request Jul 25, 2026
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.

7 participants