-
Notifications
You must be signed in to change notification settings - Fork 314
Enable deno cli #2049
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
Enable deno cli #2049
Conversation
|
View Vercel preview at instant-www-js-enable-deno-cli-jsv.vercel.app. |
| function getInstantCoreAlias(): Record<string, string> | null { | ||
| try { | ||
| const require = createRequire(import.meta.url); | ||
| const platformPath = require.resolve('@instantdb/platform'); |
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.
Maybe we should make cli require core too? imo it should be fine as they are all the same versions.
Reasoning: it will make this a bit less error prone
stopachka
left a comment
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.
Cool!
| const res = await _loadConfig(opts); | ||
| // Only use alias for Deno projects (Node projects use their own node_modules) | ||
| const projectInfo = await findProjectDir(); | ||
| const isDeno = projectInfo?.type === 'deno'; |
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.
Maybe if can write this as:
let res;
If (isDeno) {
res = await _loadConfig(...)
} else {
res = await _loadConfig
}
Reasoning: it will make clearer that importx and friends are really for isDeno
| export async function findProjectDir( | ||
| cwd?: string, | ||
| ): Promise<ProjectInfo | null> { | ||
| // Check for Deno first (more specific - if they have deno.json, they want Deno) |
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.
Is this really true? Can they have a package directory on deno?
client/packages/cli/src/index.js
Outdated
| }; | ||
|
|
||
| async function detectEnvType({ pkgDir }) { | ||
| // Check if Deno project |
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.
Is this code necc? Wouldn't we just hit L99 in the deno case?
client/packages/cli/src/index.js
Outdated
| if (!authToken) { | ||
| return; | ||
| } | ||
| return { pkgDir, instantModuleName: '@instantdb/core', authToken }; |
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.
Maybe projectInfo should be returned in this map?
I feel like it will be useful for all the children
8d79839 to
abd0312
Compare
1bb5580 to
07c4840
Compare
Problem
The Instant CLI assumes a Node.js project structure:
pkg-dir) only looks forpackage.jsonunconfig/jiti) uses Node.js module resolution, requiringnode_modules/@instantdb/coreDeno users can't use the CLI because they have
deno.jsoninstead ofpackage.json, and nonode_modules.Solution
Two changes:
deno.json/deno.jsonc@instantdb/corefrom the CLI's own dependencies--
As an aside I also included watching the cli in
make dev-- found myself needing to add this config in everytime I work on the cli and it feels like it's been frequent enough to warrant itpkg-diralso was renamed topackage-directoryrecently so updated that and also includedfind-up(which is a dependency of package-directory) to traverse find deno