shirakaba / react-nativescript

React renderer for NativeScript
https://react-nativescript.netlify.com
MIT License
280 stars 14 forks source link

Append `$` to all exports from "components" directory #37

Closed shirakaba closed 4 years ago

shirakaba commented 5 years ago

From @Lelelo1 on the Slack channel:

I have noticed when creating a custom component - then there is no differentiation beetwen $MyComponent and MyComponent. Couldn't direct access totns-core-modules/ui be restricted eventually? I have also noticed nativescript components can be imported from these 3 places currently:

import { AbsoluteLayout } from "tns-core-modules/ui/layouts/absolute-layout/absolute-layout";
import { AbsoluteLayout } from "react-nativescript/dist/components/AbsoluteLayout";
import { AbsoluteLayout } from "react-nativescript/dist/client/ElementRegistry";

My response

Regarding component naming

When creating a custom component, you can of course name it whatever you like.

For UI plugins that wrap a UI element (e.g. AbsoluteLayout), my convention is to use the name $AbsoluteLayout to refer to the component class, which prevents a name clash with the underlying UI element that it's wrapping.

For custom components that wrap other React components, there is no 'underlying component' to get a reference to, so there is no need for the $ prefix to prevent a name clash.

Regarding import paths for NativeScript core components

Yeah, the many different import paths are indeed annoying. I re-exported them via ElementRegistry for convenience (just like React Native does), but quite likely I've missed some so you can't get every module you need from there.

There is a long-standing open issue in NativeScript Core to address this.

https://github.com/NativeScript/NativeScript/issues/4041

Actionables

Good point about import { AbsoluteLayout } from "react-nativescript/dist/components/AbsoluteLayout";, though. Any component export under "react-nativescript/dist/components" currently lacks a $ prefix (which is indeed confusing). I'd better address that at some point to prevent the Intellisense auto-importing a React component when the user was intending to import a NativeScript Element...

Lelelo1 commented 5 years ago

If there could be one single import source - that export React.Component with the properties of the tns-core-modules/ui. That would be the most preferable I think.

Trying to import from tns-core-module and use in tsx:

wrong

If there is only one other import source in react-nativescript - it should not be that difficult to realise as a newcomer to pick the second (react-nativescript) import option I think. I suspect there also might be a vscode setting that can warn when importing tns-core-modules/ui

I don't know how difficult it is and will be - to have the these react-nativescript components up to date with their corresponding tns-core-modules/ui tough. If it is hard to maintain - that could be a reason to stay with $ and leave tns-core-modules/ui available.

shirakaba commented 5 years ago

If there could be one single import source - that export React.Component with the properties of the tns-core-modules/ui. That would be the most preferable I think.

Not sure I'm following you. Do you mean that you'd like to be able to type:

import { AbsoluteLayout } from "react-nativescript";

const ref: React.refObject<AbsoluteLayout> = React.createRef<AbsoluteLayout>();

<AbsoluteLayout ref={ref}/>

Because the problem there is that the React component will have a name clash with the AbsoluteLayout UI element. They can't both be called AbsoluteLayout. And since tns-core-modules/ui is already using the name AbsoluteLayout, the React component should be the one that has to be changed (e.g. with a preceding $).

Lelelo1 commented 5 years ago

Not sure I'm following you. Do you mean that you'd like to be able to type:

Yes. Right now there is the "react-nativescript" and as I just recently learned the "react-nativescript/dist/components/AbsoluteLayout" - that can't be used in react.createRef:

createRef

providing just one import source - that can then be used both in the jsx and with react.createRef - would the most preferable I think.


They can't both be called AbsoluteLayout

I might not fully understand everything that causes this limitation. But In "react-nativescript/dist/components"

import { AbsoluteLayout as NativeScriptAbsoluteLayout } from "tns-core-modules/ui/layouts/absolute-layout/absolute-layout";

And in the pull to refresh plugin example

import { PullToRefresh as NativeScriptPullToRefresh, EventData } from "@nstudio/nativescript-pulltorefresh";

... you resolve the naming conflicts. If this is in regard to the developer experience only - consider leaving it up to the developer to resolve it themselves?

shirakaba commented 5 years ago

Cause of the name clash

The following JSX compiles to the equivalent JS:

// JSX
import * as React from "react";
import { AbsoluteLayout } from "react-nativescript";

const ref: React.refObject<AbsoluteLayout> = React.createRef<AbsoluteLayout>();

<AbsoluteLayout ref={ref}/>
// JS
import * as React from "react";
import { AbsoluteLayout } from "react-nativescript";

const ref: React.refObject<AbsoluteLayout> = React.createRef<AbsoluteLayout>();

React.createElement(
    AbsoluteLayout,
    {
        ref: ref
    },
    null
);

Which is why they there's a name clash. The JSX constructor's tag name isn't merely a string; it's a reference to a real JS object (React class component).

Living with the name clash

Indeed, we could technically still name the exported JSX class component as AbsoluteLayout and simply accept the fact that developers will have to use import aliasing to resolve any name clashes when using both the JSX constructor and the NativeScript Core UI element in the same module.

But I think that designing a propensity for import name clashes into the library is unwise. The user will want to import the JSX constructors 90% of the time and only 10% of the time will want to import the NativeScript Core UI element. Yet if both had the same name, the Intellisense would be suggesting the NativeScript Core UI element 100% of the time, quite possibly as the first option (it's likely non-determinate, and in any case not everyone is using VS Code). So even if the developer knows what they're doing, it'll be frustrating.

My main resistance to this is that the repo's Issues page will very likely be filled with users time-and-time-again hitting the same JSX element type 'AbsoluteLayout' is not a constructor function for JSX elements error.

How React Native does it

<WebView ref={ref}/>

The ref.current refers to the WebView-wrapping class component (which has functions like render(), componentDidMount(), and anything else that you've added to it; I haven't really added any such functions, but React Native for instance provides goBack() on its WebView-wrapping class component) rather than the WebView itself.

You'll notice that every class component in React NativeScript includes this.myRef (inherited from Observable). So we could do this:

import * as React from "react";
import { $WebView } from "react-nativescript";
import { WebView } from "tns-core-modules/...";

// We define a WebView to be rendered like this:

<WebView ref={ref}/>

// We put this code block somewhere sensible:

const webViewClassComponentInstance: $WebView = ref.current;
const webViewNativeScriptCoreUIElement: WebView = webViewClassComponentInstance.myRef.current;
const wkWebViewInstance: WKWebView = webViewNativeScriptCoreUIElement.ios;

... Actually, I see that this doesn't solve the name clash at all. Wonder why React Native don't suffer from name clashes.

Lelelo1 commented 5 years ago

I think there might be a possibility for a react-nativescript component to be a nativescript component and react component simultaneously actually - by using mixins.

Here I have combined NativeScriptAbsoluteLayout and React.Component:

mixins

However it is a bit unclear what happens to the type when doing that.

shirakaba commented 5 years ago

I think that unioning them together is just begging for trouble!

Lelelo1 commented 5 years ago

Maybe. It is just that when working in react-native the component is the same regardless of changing it in the jsx or in componentDidMount.

shirakaba commented 5 years ago

As a concrete argument against this approach, we'd hit a hard problem if any of the method/property names on the component class happened to clash with the method/property names on the NativeScript UI element.

shirakaba commented 5 years ago

It is just that when working in react-native the component is the same regardless of changing it in the jsx or in componentDidMount.

I don't think that's true. If you set a ref on a JSX element such as WebView in React Native, it'll give you a reference to the class component. To get a reference to the underlying WebView UI element, I think you need to call something like ReactNative.findNodeHandle(this.myRef.current), which gives you a handle that you can pass to the JS-native bridge.

Fundamentally, React Native cannot directly do anything with a native element in JS; it has to pass a handle to the native bridge and ask the bridge to perform some operation on that native-managed node. As there is no concept of manipulating the native element directly from the JS VM, there are no import name clashes to worry about.

So the problem with React NativeScript is that it has to handle a feature that React Native doesn't even have :)

Lelelo1 commented 5 years ago

we'd hit a hard problem if any of the method/property names on the component class happened to clash with the method/property

Can that be handled as it is now though? If a property name clash - can I set the react property in the jsx and set the nativescript component in componentDidMount - of the same react-nativescript object simultaneously - that has the same name?


Yes I am aware of that difference. I think I meant from a developer/user experience perspective. A developer would expect being able to interact with the same class in the jsx and componentDidMount and think of it as the same thing. Especially a react developer.

So the problem with React NativeScript is that it has to handle a feature that React Native doesn't even have :)

Yes and it is wonderful it works!

shirakaba commented 5 years ago

If a property name clash - can I set the react property in the jsx and set the nativescript component in componentDidMount - of the same react-nativescript object simultaneously - that has the same name?

No, because:

  1. any NativeScript UI element members added to a class component by the mixin would override those that were previously defined on the class component (or vice versa, if you're applying the mixin the other way round).
  2. JSX doesn't have a special scope; it follows the same rules as regular JS, but with a more convenient syntax.

I think I meant from a developer/user experience perspective. A developer would expect being able to interact with the same class in the jsx and componentDidMount and think of it as the same thing. Especially a react developer.

I don't think they should expect that, though. It's well known that a React element describes a recipe for a UI hierarchy, rather than an actual reference to any instance of anything. e.g.

const element = (
  <h1 className="greeting">
    Hello, world!
  </h1>
);

... is the same as:

const element = React.createElement(
  'h1',
  {className: 'greeting'},
  'Hello, world!'
);

... which is the same as:

// Note: slightly simplified
const element = {
  type: 'h1',
  props: {
    className: 'greeting',
    children: 'Hello, world!'
  }
};

But I do agree that it would be more ergonomic to be able to write <AbsoluteLayout> as JSX whilst still being able to get a reference to the underlying AbsoluteLayout element using the name AbsoluteLayout – I just don't see how to support it without introducing an import name clash.

shirakaba commented 4 years ago

I've made some progress on this.

I studied the repo for NativeScript Core to see how they augmented global types, and learned enough to try augmenting JSX again. Here's a quick look at a new format for the index.ts file for RNS that will augment the global JSX typings.

Before

No augmentation whatsoever:

export * from "./client/ReactNativeScript";

After

We augment JSX something like this. I'm using React.DetailedHTMLProps just for the sake of example, because it's non-trivial to produce a React.DetailedNativeScriptProps.

/// <reference types="react" />
export * from "./client/ReactNativeScript";

declare global {
    namespace JSX {
        interface IntrinsicElements {
            actionBar: React.DetailedHTMLProps<React.HTMLAttributes<HTMLDivElement>, HTMLDivElement>;
            ActionBar: React.DetailedHTMLProps<React.HTMLAttributes<HTMLDivElement>, HTMLDivElement>;
        }
    }
}

You'll see I've tried augmenting the JSX typings to allow tags named <actionBar> and <ActionBar>. In practice, only the former works, however.

Here's the behaviour from the above setup:

  1. <actionBar> is now recognised as a JSX element, with all the same typings as a <div>.

  2. <ActionBar> is still not recognised as a JSX element, and instead the TypeScript compiler looks for a class component to import. I'd actually expected this behaviour – I've heard that it's convention to write all JSX primitive elements in lower case, so maybe capitalised elements aren't cross-checked against the typings of JSX IntrinsicElements.


I think we can work with this. Here's the plan that's crossing my mind:

  1. Rename all the existing (currently untyped) primitives in the element map from <label> to <_label>.

  2. Add the current class components (e.g. $Label) to the element map, but named in the map as label (resulting in the JSX tag <label>). We can rename the class components from $Label to label for consistency.

  3. Register each of those class components into the JSX IntrinsicElements map with appropriate typings (side-note: I had great trouble converting React.DetailedHTMLProps to a NativeScript equivalent last time I investigated this... it's a rabbit-hole of generics and concepts that don't map one-to-one between the Web DOM and the NativeScript UI). Here, they must each start with a lower-case letter (e.g. as actionBar is above) in order to work at all.

If we do this, users won't even need to import the RNS component classes when writing TSX. It'll be a similar experience to using react-dom 🙂

It doesn't make sense to me why React Native goes with upper-cased component names, thus forcing you to import the components. That has undoubtedly narrowed my mindset. But the fact that I've figured out how to solve a TypeScript problem that had stumped me for 7 months is the real breakthrough here. 🚀

So... next version of RNS, no more dollars, no more imports, and even a bit less usage of the shift key 😲

rizrmd commented 4 years ago

So... next version of RNS, no more dollars, no more imports, and even a bit less usage of the shift key

^ This, I would like to help if I could.

shirakaba commented 4 years ago

@rizkyramadhan Unfortunately I investigated all of these options and found that none of them would work in practice. Primitive components (starting with lower case letters) cannot have the sophistication of a React class component – and we need that sophistication for things like event listener handling and inheritance. Fundamentally, NativeScript Core itself needs to stop naming the modules with the same names that React NativeScript class components use – otherwise we'll get endless namespace clashes and incorrect auto-imports 😕

shirakaba commented 4 years ago

@rizkyramadhan For your own usage, you can easily make your own helper file that re-exports every class component (beginning with a $) as one that doesn't begin with a $ and see what the dev experience is like. In practice, I think it would be too infuriating and involve a lot of fighting with the IDE.

Thank you for your offer to help, though, it means a lot to me 🙂 I can't see a way forward on this problem though, unfortunately.

rizrmd commented 4 years ago

@rizkyramadhan Unfortunately I investigated all of these options and found that none of them would work in practice. Primitive components (starting with lower case letters) cannot have the sophistication of a React class component – and we need that sophistication for things like event listener handling and inheritance. Fundamentally, NativeScript Core itself needs to stop naming the modules with the same names that React NativeScript class components use – otherwise we'll get endless namespace clashes and incorrect auto-imports 😕

Maybe we can prepend our classes with RN instead of $, since naming React Component with $ is just... weird?

FYI, I just learnt that we could name react component starting with $ from this library

shirakaba commented 4 years ago

Maybe we can prepend our classes with RN instead of $, since naming React Component with $ is just... weird?

@rizkyramadhan I considered it, but it's more characters to type! Inputting '$' immediately gets you Intellisense for a very small set of imports, whereas inputting 'R' followed by 'N' is going to slow you down by a few (hundred?) milliseconds each time as it does lookups for imports starting with 'R', then 'RN'. Not fun.

FYI, I just learnt that we could name react component starting with $ from this library

😄 It was a learning experience for me, too! Another easily typed non-alphabetic character, _, is available, too. You can use any valid identifier name in JS, as long as it doesn't start with a lower-case letter (which TSX – and likely also JSX – interprets as a primitive).

It looks like we could use π, λ, or even ლ_ಠ益ಠ_ლ or Hͫ̆̒̐ͣ̊̄ͯ͗͏̵̗̻̰̠̬͝ͅE̴̷̬͎̱̘͇͍̾ͦ͊͒͊̓̓̐_̫̠̱̩̭̤͈̑̎̋ͮͩ̒͑̾͋͘Ç̳͕̯̭̱̲̣̠̜͋̍O̴̦̗̯̹̼ͭ̐ͨ̊̈͘͠M̶̝̠̭̭̤̻͓͑̓̊ͣͤ̎͟͠E̢̞̮̹͍̞̳̣ͣͪ͐̈T̡̯̳̭̜̠͕͌̈́̽̿ͤ̿̅̑Ḧ̱̱̺̰̳̹̘̰́̏ͪ̂̽͂̀͠, too! But they're considerably harder to type out.

rizrmd commented 4 years ago

Well, code is often read than wrote. I think readable code will be good investment over several ms writing it. Just my 2c :)

shirakaba commented 4 years ago

React NativeScript v1 solves all of this!

import * as React from "react";
import { WebView } from "@nativescript/core";

function BrowserExample({}){
  const myRef = React.useRef<WebView>(null);

  return (
    <webView
      ref={myRef}
      width="100%"
      height="100%"
      src={"https://react-nativescript.netlify.app"}
    />
  );
}