Review from a developer familiar with Vue

[dnischeta]

Introduction

@alexgton asked me to review that project from a developer familiar with Vue perspective. So that issue contains review summary.

Issues are grouped using next strategy:

• issues are grouped by priority:
  • Critical - issues which are blocking for public release.
  • Moderate - issues which may lead to bugs or errors.
  • Minor - other issues (Vue practices violation, potential performance degradation, code-style, etc.).
• inside priority group issues are classified by type:
  • Usage - issues related to public interface of logic which library exports to the user.
  • Implementation - issues related to the implementation (reactivity, potential bugs, Vue-way, etc.).
  • Documentation - issues related to the docs.
  • Tooling/Dependencies - issues related to the dependencies and project configuration.

How review was done

1. Project source code inspection.
2. Testing as a potential user (I've deployed demo apps):
   • Vue@2.7 demo
   • Vue@3 demo

Critical issues

[Critical] usage

1. <TonConnectButton> props styles and className should be removed. Vue binds style and class props to the root element by default, see docs. So it's better to remove redundant props before public release.
2. README.md should be updated and synchronized with final public interface. I've found a few inconsistent examples:
   • it says that TonConnectButton has style and class props, but actually it has only styles and className props. (in that cade, actually, README is correct, see Usage - TonConnectButton).
   • wrong useTonConnectUI examples. Examples say, that user should use const tonConnectUI = useTonConnectUI(); but actually user should do const { tonConnectUI } = useTonConnectUI(); either const [tonConnectUI] = useTonConnectUI(); depending on final implementation.

[Critical] implementation

1. TonConnectUI DI issue. Now, <TonConnectUiProvider> won't recreate TonConnectUi instance on options prop change, React implementation will. Probably, that should be fixed in <TonConnectUiProvider>. Next step is providing reactive object instead of raw:

   
   // <TonConnectUIProvider />
   const tonConnectUI = shallowRef<TonConnectUI | null>(null);

provide('ton-connect-ui', tonConnectUI);

// Injection consumer, for example useTonWallet const tonConnectUi = inject('ton-connect-ui'); // That line of code should be inside useTonConnectUI, put it here for simplification const unsub = shallowRef<() => void>(() => {}); const wallet = shallowRef<...>(null);

onMounted(() => { watch(tonConnectUI, (actualTonConnectUI) => { unsub(); // Unsubscribe on watcher trigger <-> when tonConnectUI instance is updated (as done in React)

wallet.value = actualTonConnectUI.wallet;

unsub.value = actualTonConnectUI.onStatusChange((value) => {
  wallet.value = value;
});

}); });

onUnmounted(() => { unsub.value(); });

And injections usage should be updated also, injections components should react on potential tonConnectUi changes.

### [Critical] tooling/dependencies

1. Isomorphic build using `vue-demi`.

    Project should clarify which Vue versions are supported. Then get rid of unused dependencies and configurations. 

    Also, I don't understand current `vite.config`, why don't make `vue-demi` extenal? **I'm not an expert in vue-demi usage** but after some research I found that libs built with vue-demi have vue-demi imports inside bundled code and actual Vue version defined after user installed pacakge. See [@vueuse/core](https://www.npmjs.com/package/@vueuse/core?activeTab=code) for example.

    On the other hand, tonconnect-ui-vue build output targets specific Vue version, I bet, it may become a trouble in case Vue < 2.7 support is required.

1. Remove redundant lock-file

    For me, developer from the outside the project, it was a complicated to choose which package manager should be used to work with the project. So either `package-lock.json` or `yarn.lock` should be removed. Also, I think a fresh package manager version should be used (eg. yarn.lock generated using old version of yarn).

## Moderate issues

### [Moderate] usage

1. Props should be types properly, to do it you should configure component as `defineComponent<{ options: TonConnectUIProviderProps }>`. For example `TonConnectProvider.vue` doesn't have typed props, see: 
![image (8)](https://github.com/user-attachments/assets/d42c93eb-9d70-472f-9940-4d82be0b5a6c)
1. Debug `console.log()`s should be removed I believe.

### [Moderate] implementation

1. `provide`/`inject` usage
    - Symbol key should be used to avoid potential collisions with user apps, see [docs](https://vuejs.org/guide/components/provide-inject.html#working-with-symbol-keys).
    - `InjectionKey` should be used to improve provide-inject typing, see [docs](https://vuejs.org/guide/typescript/composition-api.html#typing-provide-inject).
1. [`<TonConnectUiProvider>`](https://github.com/TownSquareXYZ/tonconnect-ui-vue/blob/main/src/components/TonConnectUIProvider.vue#L35): remove `render` field. `vue-demi` for both Vue@3 and Vue@2.7 will use function returned from `setup`.
1. [`<TonConnectUiProvider>`](https://github.com/TownSquareXYZ/tonconnect-ui-vue/blob/main/src/components/TonConnectUIProvider.vue#L35): no need to render string "nothing" in case user didn't pass default slot.
1. [`<TonConnectUiProvider>`](https://github.com/TownSquareXYZ/tonconnect-ui-vue/blob/main/src/components/TonConnectUIProvider.vue#L35): no need to use `isVue2` inside render:
    ```typescript
      return () => {
        return h(
          "div",
          slots.default?.() ?? null
        );
      };

1. Remove unused code from the project.
2. Remove react references (I bet it's here because project was started using @tonconnect/ui-react).

Minor issues

[Minor] Usage

1. useTonConnectUI returns tuple [ui, setOptions] similar to react implementation. But developer familiar with Vue would expect that interface const { tonConnectUI, setOptions } = useTonConnectUI(). It's not a common practice in Vue to return tuples but that issue only affects code style.

[Minor] implementation

1. ShallowRefs

   ShallowRef may be used to wrap TonConnectUI or Wallet instances to escape redundant calculations.

2. computed instead of watchEffect

   In useTonAddress computed should be used instead of watchEffect. watchEffect should be used in case some side-effect is required. For derivative state (as address of the wallet) computed should be used.

3. Props: required: false should be omitted

   In TonConnectButton optional props are marked with required: false. This is redundant, props is optional by default.

4. Native elements attrs

   In TonConnectButton.

   No need to bind id as attr, it's enough to bind it explicitly. So binding: attrs: { id: props.buttonRootId || "" } should be removed.

5. Configure formatting/linting in CI. Now there are a couple of places with inconsistent formatting.

6. /scripts/ directory should be checked, postinstall script does nothing.

Conclusion and recommendation

I've checked that library in vue@2.7 and vue@3 project. Looks like it works fine in common scenarios. But I would recommend to fix or discuss and decline all issues mentioned in critical and moderate groups before public release.

[sansx]

Hey @dnischeta, thanks for your insightful review and suggestions! 👍 We've addressed the issues in the updated version here #10 . Could you please take a look and let us know if it's ready to be merged?

[dnischeta]

@sansx , hello! I'll take a look today.

[dnischeta]

One other thing I was thinking about is adding a plugin. That solution replaces need of <TonConnectUIProvider> with installing that plugin, user will do:

import { createApp } from 'vue'

const app = createApp({})

app.use(tonConnectUI, {
  manifestURL: '...'
})

tonConnectUI plugin may register a global property or use provide.