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

Test the packaged version of the extension on CI and get it working #233

Merged
merged 13 commits into from
Feb 5, 2022

Conversation

rictic
Copy link
Collaborator

@rictic rictic commented Feb 4, 2022

No description provided.

@rictic
Copy link
Collaborator Author

rictic commented Feb 4, 2022

With this, the .vsix file works for me. Turns out the issue was that vscode-web-custom-data couldn't be resolved when loading the extension. Opening the TS server log has been super helpful for debugging.

We don't yet have an integration test of loading the .vsix file, but I think I'm fairly close to something kinda close that should work. The trick is to package up the vsix file, rename it to a zip file, unzip the extension folder from within it, and then move that to a directory entirely outside of the repo filesystem entirely (so that recursive node_module resolution doesn't pick up deps that might be in the repo but not in the package), then point the extensionDevelopmentPath at that directory.

@justinfagnani
Copy link
Collaborator

nice sleuthing!

wrt unzipping out of the repo tree... you should also be able to put it in a subfolder. Node resolution looks up for any ancestor containing a node_modules folder, but not to any cousin. So repo/temp/node_modules won't be looked at.

This packages up the extension, then unpackages it into a grandparent directory and uses that as the extension to test.
@rictic
Copy link
Collaborator Author

rictic commented Feb 5, 2022

Ah, I tried putting it in the root dir of the repo, and the problem is that vscode-web-custom-data is present in the root node_modules directory! Not sure what's doing that, maybe some clever lerna optimization?

@rictic
Copy link
Collaborator Author

rictic commented Feb 5, 2022

Ha ha! Moving it outside the repo entirely (I know, it's gross, and a violation of the implicit boundary) successfully reproduced the error that we've been seeing where the .visx file doesn't work! https://github.com/runem/lit-analyzer/runs/5074481601?check_suite_focus=true

And then adding the vscode-web-custom-data dep back in gets the same test passing! https://github.com/runem/lit-analyzer/runs/5074504695?check_suite_focus=true

@rictic rictic changed the title Some improvements to extension packing Test the packaged version of the extension on CI Feb 5, 2022
@rictic rictic changed the title Test the packaged version of the extension on CI Test the packaged version of the extension on CI and get it working Feb 5, 2022
@rictic rictic marked this pull request as ready for review February 5, 2022 00:40
@justinfagnani
Copy link
Collaborator

Ah, I tried putting it in the root dir of the repo, and the problem is that vscode-web-custom-data is present in the root node_modules directory! Not sure what's doing that, maybe some clever lerna optimization?

No, there's a custom copy step that I would like to get rid of

@rictic
Copy link
Collaborator Author

rictic commented Feb 5, 2022

I think it might be lerna bootstrap --hoist

@rictic
Copy link
Collaborator Author

rictic commented Feb 5, 2022

but in any case, putting it outside the repo itself is a more hermetic option anyways

@justinfagnani
Copy link
Collaborator

oh, oh... there is a copy step for the first-party extension. I thought that's what you were talking about. Yeah, hoist would do this here. It'd be good to turn hoisting off for the extension, and if that's not possible, maybe just do it for the whole repo. I had added more packages to nohoist, but it seems like it might be a game of whack-a-mole.

Copy link
Collaborator

@justinfagnani justinfagnani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@rictic rictic merged commit 72d0049 into master Feb 5, 2022
@rictic rictic deleted the test-packaged branch February 5, 2022 03:48
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.

2 participants