-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Support multiple Relay project configurations in extension #4238
base: main
Are you sure you want to change the base?
Support multiple Relay project configurations in extension #4238
Conversation
@@ -0,0 +1 @@ | |||
16 |
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.
Let me know if there's a reason we don't want to do this. When I tried running yarn
from the root of the repo on Node 18 I was getting
[15:30:49] 'bundleTask' errored after 118 ms
[15:30:49] Error: error:0308010C:digital envelope routines::unsupported
at new Hash (node:internal/crypto/hash:71:19)
at Object.createHash (node:crypto:133:10)
Pretty sure it's a webpack problem with the version we're on: webpack/webpack#14532
package.json
Outdated
@@ -20,9 +20,6 @@ | |||
"test-dependencies": "node ./scripts/testDependencies.js", | |||
"typecheck": "flow check" | |||
}, | |||
"workspaces": [ | |||
"vscode-extension" | |||
], |
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.
Using yarn workspaces wrecks havoc with the VSCode extension publishing tool. It fails to find the right dependencies, duplicates deps, etc. I'm not sure if there was benefit we were getting from this, but removing it makes the bundle/publishing of the extension much easier.
vscode-extension/tsconfig.json
Outdated
@@ -5,7 +5,7 @@ | |||
"lib": ["es2020"], | |||
"outDir": "out", | |||
"rootDir": "src", | |||
"sourceMap": true, | |||
"sourceMap": false, |
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.
No need to generate sourcemap files within the compiled extension output
vscode-extension/package.json
Outdated
"build-local": "vsce package", | ||
"esbuild-base": "esbuild ./src/extension.ts --bundle --outfile=out/extension.js --external:vscode --format=cjs --platform=node", | ||
"esbuild": "npm run esbuild-base -- --sourcemap", | ||
"esbuild-watch": "npm run esbuild-base -- --sourcemap --watch" |
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.
Pretty much just copied this from the VSCode docs https://code.visualstudio.com/api/working-with-extensions/bundling-extension#using-esbuild
I don't really understand the unit test failures at this point since my changes shouldn't have touched any of that. Maybe the cargo.lock file needs to be updated, not sure. |
); | ||
|
||
terminal.show(); |
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 disables the terminal auto-showing when switching to a workspace that starts the compiler by default. I don't see much reason to auto-show this when switching workspaces and personally find it a bit annoying
…on to the new relay.projects config. Just map the current configs to a project structure internally
Summary: This ports over a subset of the changes from [this PR](#4238) that just fix some build issues with the VSCode extension. This works to build the extension locally, which should in theory also work with the Github Action publish step, but I can't fully verify that since it requires the marketplace PAT to run. However, we **should not** do a publish after this PR merges because the current tagged version is 2.0.0 and we should wait for the other PR to land before we publish, especially since we're not going to publish 2.0.0 and instead are planning on publishing version 1.3.0. Pull Request resolved: #4275 Reviewed By: josephsavona Differential Revision: D45661618 Pulled By: captbaritone fbshipit-source-id: 321f49060ea15608bbb85c85a68de52cdf37b97d
Any update on that PR? What do we need to finish and merge this PR? |
Do you plan to release this Pull Request? I added relay.projects to settings.json as documented, but it did not work correctly and upon investigation I noticed that this PR has not yet been merged. |
I just came looking to find out why that wasn't working, since it was documented as a feature. Do we know when this would be released? |
Same for me, can we merge this MR and do a release ? Required for mono repo :) |
What is everyone doing as a workaround to handle monorepos with multiple relay configs? |
You can pass projects into your module.exports = {
root: ".",
sources: {
// put here the source of each project, the root dir of each package of your monorepo
},
projects: {
project1: {
schema: "...",
language: "typescript",
},
// ...
},
} |
@noghartt thank you! |
Reviving #3972 into a separate PR and taking over for Terence. This mostly just copies that PR into a new one, but also does a few other fixes
vsce
. I updated to the non-deprecated@vscode/vsce
updated package. Packaging/publishing has changed a bit nowrelay.projects
configuration. Also added a section to the README on how to migrate from v1 to v2I tested this extension in a handful of different projects, some with no configuration needed, some with the old configuration options (which we map to the new one), and workspaces with multiple projects. However, if anyone has the time I'd love to have others try it out. Just run
yarn build-local
from thevscode-extension
directory and manually install the resultingrelay-2.0.0.vsix
file into VSCode.Unrelated to the core purpose of this change, but I also added a
.nvmrc
file to the root of this repo (and removed.nvmrc
from the gitignore since runningyarn
at the root of this repo with Node 18 fails. Not sure if we were intentionally not including .nvmrc files though, so I can revert if needed./cc @davidmccabe @captbaritone @tbezman