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

[FEATURE] Consider Alternative Implementations for cause in ErrorOptions to Maintain Lower Compilation Target Compatibility #956

Closed
andreas-karlsson opened this issue Jun 6, 2024 · 4 comments · Fixed by #957
Closed

[FEATURE] Consider Alternative Implementations for cause in ErrorOptions to Maintain Lower Compilation Target Compatibility #956

andreas-karlsson opened this issue Jun 6, 2024 · 4 comments · Fixed by #957
Assignees
@beeme1mr
Labels
enhancement New feature or request

Comments

@andreas-karlsson
Copy link

Requirements

A while back, PR #765 was merged, introducing support for the cause property in ErrorOptions. This was implemented by changing the compilation target from es2015 to es2022. However, this change may cause compilation errors for SDK users who are compiling to a lower target and have skipLibChecks:false.

While the cause functionality is a valuable addition, I'm concerned about the potential impact on runtime support. Is the added functionality worth the potential decrease in compatibility?

I'm wondering if there might be alternative ways to implement this feature without changing the compilation target. For instance, could we manually set the cause property where needed? This approach might be compatible with both older and newer runtimes, and could potentially avoid the issue altogether.

I'm interested in hearing your thoughts, and open to contributing a PR.
@andreas-karlsson andreas-karlsson added the enhancement New feature or request label Jun 6, 2024
@beeme1mr
Copy link
Member

beeme1mr commented Jun 7, 2024

We made this change because Node 16 has solid support for es2022 and it's the oldest LTS version still supported. However, you mentioned that it may cause issues when targeting es2015 when skipLibChecks: false is set. I believe es2015 is still the default when you run ts init, so we may want to try and target that, if possible. Perhaps a polyfill would be enough.

What do you think @lukas-reining @toddbaert?

@lukas-reining
Copy link
Member

This makes sense @andreas-karlsson.
I think we could go for a polyfill here @beeme1mr, I am just not sure if this works for TS types.

@beeme1mr beeme1mr self-assigned this Jun 10, 2024
@beeme1mr beeme1mr linked a pull request Jun 10, 2024 that will close this issue
feat: lower compilation target to es2015 #957
Merged
@beeme1mr beeme1mr mentioned this issue Jun 10, 2024
Merged
@beeme1mr
Copy link
Member

I decided not to use a polyfill because it wasn't necessary and may lead to unexpected consequences.

github-merge-queue bot pushed a commit that referenced this issue Jun 11, 2024
@beeme1mr
feat: lower compilation target to es2015 (#957)
c2d6c17 
## This PR

- replace the es2022 error cause with a custom implementation
- lower compilation target from es2022 to es2015

### Related Issues

Fixes #956

### Notes

The tests pass, but I still want to manually build and test the outputs
in a real application to ensure everything works as expected.

Signed-off-by: Michael Beemer <[email protected]>
@andreas-karlsson
Copy link
Author

Thanks for speedy resolution! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@beeme1mr beeme1mr

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: lower compilation target to es2015 open-feature/js-sdk
3 participants
@andreas-karlsson @beeme1mr @lukas-reining