-
-
Notifications
You must be signed in to change notification settings - Fork 222
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
Use cljs for state #1053
Use cljs for state #1053
Conversation
@PEZ I'm having a pretty odd issue that I can't yet figure out. It seems that any function from the cljs-lib called inside the namespace.ts function
This originally was happening for the new This branch has my troubleshooting code with just minimal changes from dev. If you could take a look and see if you see anything obvious that would be great. This is really odd to me, especially since |
You can see in the Log (Extension Host) output the stack trace of the error when trying it out. I also noticed that if I move the calls to the cljs functions up one level in the call stack to state.ts/ |
This is really odd. If I copy all of namespace.ts into another file, namespace2.ts, and I change the status.ts function update(context = state.extensionContext) {
vscode.commands.executeCommand('setContext', 'calva:showReplUi', shouldshowReplUi(context));
namespace2.updateREPLSessionType();
//namespace.updateREPLSessionType();
statusbar.update(context);
} The call to
|
Just noticed it works fine when I package it as a vsix and install it, it just doesn't work in dev 🤔. Must be something weird with bundling/compiling going on. |
Just want to add here that this problem occurs even on |
…ding/writing to calling code
Refactor calls to update repl session type to use new function.
@PEZ I've worked around the above issue by a little refactoring. I also posted an issue in the shadow repo about it to see if Thomas has any insights, but I think we can move forward now. If you wouldn't mind looking over these changes and letting me know if you'd like any changes that would be great. Definitely want to test this a good bit before merging. I also still need to split up the state module / refactor it into other modules. I don't plan to change the code there though. I want to change as little as needed for this PR, but I think leaving the state.ts module named / grouped as it is could be misleading. |
Close #1052.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great stuff. I have a few nitpicks, that's all.
Co-authored-by: Peter Strömberg <pez@pezius.com>
…lva into use_cljs_for_state
What has Changed?
The cljs lib issue
In two files that I've seen so far, annotations.ts and namespace.ts, if we import the cljs lib directly and call functions from it, during runtime we get an error that the function is not defined, even though they work in many other files. If we bundle the extension for release and run the vsix, it works fine. Something is not being linked correctly, but I have not figured out the root of the issue.
The workaround
I've found that if we export the cljs lib from utilities.ts, then import it from utilities.ts in those troublesome files noted above, the function calls work fine during runtime. I tried to make a separate cljs-lib.ts and import+export it from there, then import the lib from that module, but again it doesn't work. Since I know it works from the utilities module I left that workaround in place for now.
Maybe later we can figure out the root issue, but at some point it might not matter anyway. In the meantime I'm not sure if we should make all imports of the cljs lib come from the utilities module just to be safe. There are many places in the code where we call functions from the cljs lib now since we use it for state, and I haven't yet verified every single one of those calls works. If we discover some don't, we can switch it to be imported from the utilities module.
Closes #1052
My Calva PR Checklist
I have:
dev
branch. (Or have specific reasons to target some other branch.)published
. (Sorry for the nagging.)[Unreleased]
entry inCHANGELOG.md
, linking the issue(s) that the PR is addressing.ci/circleci: build
test. NB: There is a CircleCI bug that makes the Artifacts hard to find. Please see this issue for workarounds.The Calva Team PR Checklist:
Before merging we (at least one of us) have:
dev
branch (unless reasons).Ping @PEZ, @kstehn, @cfehse, @bpringe