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.

Sign up for GitHub

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

Jump to bottom

env=browser treated same as env=both. Causes globalThis.Buffer typescript error #967

Closed
davidfiala opened this issue Nov 21, 2023 · 5 comments · May be fixed by Graysonbarton/synthetics-sdk-nodejs#2
Closed

env=browser treated same as env=both. Causes globalThis.Buffer typescript error #967

davidfiala opened this issue Nov 21, 2023 · 5 comments · May be fixed by Graysonbarton/synthetics-sdk-nodejs#2

Comments

@davidfiala
Copy link

davidfiala commented Nov 21, 2023 •
edited
Loading

When outputting with env=browser you get the same output as env=both.

This is generally OK, except when you have strict typescript checks on. You end up in a situation where the generated code includes a line like: if (globalThis.Buffer) to see if Buffer exists. This makes sense for env=both, but will cause typescript errors on a browser environment because globalThis type does not include Buffer.

As a workaround, we can enable: both of: env=browser,globalThisPolyfill=true.

However, it seems to me that a cleaner approach might be to have the env enum disable checks for globalThis.Buffer altogether. In searching through today's ts-proto code, I cannot see any cases where the env is checked against anything except node. Thereby accidentally making both equivalent to browser, which is probably not the original intent.

Some proposals:

  1. preferred: env=browser will stop checking for globalThis.Buffer
  2. If env=browser is detected, then we auto-enable the polyfill.
@davidfiala davidfiala changed the title env=browser does nothing, and will yield typescript any errors because globalThis.Buffer is not defined env=browser does nothing, and will yield typescript any errors because globalThis.Buffer is not in browser types Nov 21, 2023
@davidfiala davidfiala changed the title env=browser does nothing, and will yield typescript any errors because globalThis.Buffer is not in browser types env=browser treated same as env=both. Causes globalThis.Buffer typescript error Nov 21, 2023
cmd-johnson added a commit to cmd-johnson/ts-proto that referenced this issue Feb 13, 2024
@cmd-johnson
fix: don't reference globalThis.Buffer when env=browser (stephenh#967)
93f79af 
The generated bytesFromBase64 and base64FromBytes functions now only include code that's required for the specified env:
For env=node, the functions now exclusively use globalThis.Buffer to de-/encode to and from JSON
For env=browser, globalThis.btoa/atob is used
For env=both, the two functions use either the node or browser implementations depending on whether globalThis.Buffer exists
cmd-johnson added a commit to cmd-johnson/ts-proto that referenced this issue Feb 13, 2024
@cmd-johnson
fix: don't reference globalThis.Buffer when env=browser (stephenh#967)
31583a0 
The generated bytesFromBase64 and base64FromBytes functions now only include code that's required for the specified env:
For env=node, the functions now exclusively use globalThis.Buffer to de-/encode to and from JSON
For env=browser, globalThis.btoa/atob is used
For env=both, the two functions use either the node or browser implementations depending on whether globalThis.Buffer exists
cmd-johnson added a commit to cmd-johnson/ts-proto that referenced this issue Feb 13, 2024
@cmd-johnson
fix: don't reference globalThis.Buffer when env=browser (stephenh#967)
31ff70d 
The generated bytesFromBase64 and base64FromBytes functions now only include code that's required for the specified env:
For env=node, the functions now exclusively use globalThis.Buffer to de-/encode to and from JSON
For env=browser, globalThis.btoa/atob is used
For env=both, the two functions use either the node or browser implementations depending on whether globalThis.Buffer exists
cmd-johnson added a commit to cmd-johnson/ts-proto that referenced this issue Feb 13, 2024
@cmd-johnson
fix: don't reference globalThis.Buffer when env=browser (stephenh#967)
5fc1448 
The generated bytesFromBase64 and base64FromBytes functions now only include code that's required for the specified env:
For env=node, the functions now exclusively use globalThis.Buffer to de-/encode to and from JSON
For env=browser, globalThis.btoa/atob is used
For env=both, the two functions use either the node or browser implementations depending on whether globalThis.Buffer exists
@cmd-johnson cmd-johnson mentioned this issue Feb 13, 2024
Merged
@jonaskello
Copy link
Collaborator

For me ts-proto generates code like this:

if (globalThis.Buffer)

Which leads to typescript error:

error TS7017: Element implicitly has an 'any' type because type 'typeof globalThis' has no index signature.

I want to support both node and browser with the same code. What would be the correct setting?

cmd-johnson added a commit to cmd-johnson/ts-proto that referenced this issue Feb 15, 2024
@cmd-johnson
fix: don't reference globalThis.Buffer when env=browser (stephenh#967)
743553d 
The generated bytesFromBase64 and base64FromBytes functions now only include code that's required for the specified env:
For env=node, the functions now exclusively use globalThis.Buffer to de-/encode to and from JSON
For env=browser, globalThis.btoa/atob is used
For env=both, the two functions use either the node or browser implementations depending on whether globalThis.Buffer exists
stephenh pushed a commit that referenced this issue Feb 15, 2024
@cmd-johnson
fix: don't reference globalThis.Buffer when env=browser (#967) (#999)
0d34612 
The generated bytesFromBase64 and base64FromBytes functions now only include code that's required for the specified env:
For env=node, the functions now exclusively use globalThis.Buffer to de-/encode to and from JSON
For env=browser, globalThis.btoa/atob is used
For env=both, the two functions use either the node or browser implementations depending on whether globalThis.Buffer exists
@stephenh
Copy link
Owner

I want to support both node and browser with the same code. What would be the correct setting?

@jonaskello I guess for env=both, we could make the snippet be if ((globalThis as any).Buffer) and that will remove the TS warning.

If you'd like to submit a PR to do that, that'd be great!

I'm going to close this issue since #999 fixes the behavior with env=browser.

stephenh pushed a commit that referenced this issue Feb 15, 2024
@semantic-release-bot
chore(release): 1.167.4 [skip ci]
14f4635 
## [1.167.4](v1.167.3...v1.167.4) (2024-02-15)

### Bug Fixes

* don't reference globalThis.Buffer when env=browser ([#967](#967)) ([#999](#999)) ([0d34612](0d34612))
@jonaskello
Copy link
Collaborator

Got it. If I can find the time I'll make a PR for that.

I was able to work around it for now. We have all the generated files in a separate package with it's own tsconfig so I could disable the strict any setting for those files without affecting the rest of the app.

stephenh added a commit that referenced this issue Feb 17, 2024
@stephenh
fix: Use as any on globalThis.Buffer check.
05dff40 
See #967.
@stephenh stephenh mentioned this issue Feb 17, 2024
Merged
stephenh added a commit that referenced this issue Feb 17, 2024
@stephenh
fix: Use as any on globalThis.Buffer check. (#1004)
11d06b4 
See #967.
@stephenh
Copy link
Owner

@jonaskello it was an easy fix, so I went ahead and pushed out a change to add the as any to the globalThis.Buffer check.

stephenh pushed a commit that referenced this issue Feb 17, 2024
@semantic-release-bot
chore(release): 1.167.7 [skip ci]
308d27a 
## [1.167.7](v1.167.6...v1.167.7) (2024-02-17)

### Bug Fixes

* Use as any on globalThis.Buffer check. ([#1004](#1004)) ([11d06b4](11d06b4)), closes [#967](#967)
@jonaskello jonaskello mentioned this issue Feb 18, 2024
Merged
@jonaskello
Copy link
Collaborator

@stephenh Thanks for the quick fix. I tried 1.167.7 and it works for the base64FromBytes function but not for the bytesFromBase64 function. So I created PR #1005 with the same fix for that function.

stephenh pushed a commit that referenced this issue Feb 18, 2024
@jonaskello
fix: Use as any on globalThis.Buffer check for bytesFromBase64 (#1005)
bae741c 
This is a follow up PR for #1004 which also add `as any` for the
`bytesFromBase64` function to make it work fully with strict tsconfig
setting. See #967.
stephenh pushed a commit that referenced this issue Feb 18, 2024
@semantic-release-bot
chore(release): 1.167.8 [skip ci]
ba97f98 
## [1.167.8](v1.167.7...v1.167.8) (2024-02-18)

### Bug Fixes

* Use as any on globalThis.Buffer check for bytesFromBase64 ([#1005](#1005)) ([bae741c](bae741c)), closes [#1004](#1004) [#967](#967)
@Graysonbarton Graysonbarton mentioned this issue May 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Snyk] Upgrade ts-proto from 1.148.1 to 1.173.0 Graysonbarton/synthetics-sdk-nodejs
3 participants
@stephenh @jonaskello @davidfiala