Skip to content

Suite of cli and perms bugfixes #627

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

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Suite of cli and perms bugfixes #627

merged 1 commit into from
Dec 11, 2024

Conversation

stopachka
Copy link
Contributor

Made a series of tweaks to CLI, from what I noticed from the user session with Zack.

Small changes

  1. npx instant-cli init now detects expo, and shares the env var for expo (EXPO_PUBLIC...)
  2. We fixed a bug where if the user had no rules, we generated a perms file with an erroneous semicolon.
  3. Added instant-cli to react-native-expo, so we can test it there too.
  4. Made $default an allowed key in InstantRules.ts, and in Perms Editor
  5. Made it so $default is no longer a reserved name on the backend

Controversial Change: replace imports for @instantdb/react-native

I realized that CLI has been broken for react-native. The main reason, was that when we ran loadConfig, we would evaluate instant.schema.ts and instant.perms.ts in a node environment.

Inside the file, we would improve from our library like so:

import { i } from '@instantdb/react-native' 

The problem is, @instantdb/react-native imports react-native libraries, which fail in node.

There were a few options I thought of for getting around this:

A) Make @instantdb/core a peer

We could make @instantdb/core a peer library, instead of a dependency. We could then import { i } from '@instantdb/core'. This works great, but it would now force the user to install two libraries to get going. i.m.o that's a bit of a sadder experience. Perhaps it's worth it down the line, but I didn't want support for CLI to be the reason we switch.

B) Conditional entry points

We could have detected whether we were in a CLI environment, and had @instantdb/react-native run different entry points. I thought this would be changing the library too much though, just for CLI

C) Have a separate entry point for CLI

We could have a cli.ts file, which doesn't import react-native code. We could either tell the user to use it explicitly, or to just replace calls of @instantdb/react-native with @instantdb/react-native/dist/cli. I ended up doing this replace approach.

@dwwoelfel @nezaj @tonsky

Copy link

View Vercel preview at instant-www-js-w1-jsv.vercel.app.

Copy link
Contributor

@dwwoelfel dwwoelfel left a comment

Choose a reason for hiding this comment

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

3 seems like the best approach to me too.

LGTM!

@stopachka stopachka merged commit 1903d13 into main Dec 11, 2024
27 checks passed
@stopachka stopachka deleted the w1 branch December 11, 2024 19:55
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