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

feat: add hasEnv, throw when getEnv finds no env #2039

Closed
wants to merge 6 commits into from

Conversation

coolsoftwaretyler
Copy link
Collaborator

This PR should close #820, and is built off the initial work done by @t49tran in #938. I'm just opening this PR to re-implement on top of the master branch, rather than rebasing directly there.

There are a few differences between the diff on the initial PR and this one:

  1. I ran yarn build-docs, so there are a lot more documentation changes in the diff.
  2. I added a few additional expectations in packages/mobx-state-tree/__tests__/core/env.test.ts where I thought it might be relevant to check how hasEnv and getEnv work with these changes. There was also one test that had to be slightly modified that I don't think existed at the time of throw error when env not found #938.
  3. I had to make a few more modifications to packages/mst-middlewares/src/undo-manager.ts than the original author did, since that middleware has changed over time.

@coolsoftwaretyler
Copy link
Collaborator Author

@jamonholmgren - when you get a chance, will you please take a look at this PR to add some error-throwing behavior to getEnv in MST? This should close #938, which you were looking for volunteers on a few years ago.

Based on our conversation about stability, do you think making this change still makes sense? Seems like a breaking change and would require a new major version.

I mostly wanted to get some small code-changing experience in the repo, and that old PR seemed like a good place to start. I found this work instructive, so even if we close both PRs, I won't be bummed about it or anything.

Thanks in advance!

@jamonholmgren jamonholmgren force-pushed the feat/throw-error-when-env-not-found branch from 48559d4 to 216991a Compare July 11, 2023 04:01
@jamonholmgren jamonholmgren added breaking change Breaking change next-release Ready to go out in next release labels Jul 11, 2023
@jamonholmgren
Copy link
Collaborator

This will get released in the next breaking release.

@coolsoftwaretyler
Copy link
Collaborator Author

We're probably going to do a breaking release soon, so I will prepare this MR to merge soon! Thanks for bearing with us!

@coolsoftwaretyler
Copy link
Collaborator Author

Ok, one more attempt! Closing in favor of #2163

@coolsoftwaretyler coolsoftwaretyler deleted the feat/throw-error-when-env-not-found branch March 10, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Breaking change next-release Ready to go out in next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make getEnv fail when root has not been initialized with context
2 participants