Yomguithereal / react-blessed

A react renderer for blessed.
MIT License
4.45k stars 177 forks source link

plan b to solve tag conflict #86

Closed guoshencheng closed 5 years ago

guoshencheng commented 5 years ago

Use <blessed-xxx / > instead of <xxx /> And add a typing definition PR at DefinitelyTyped

To be honest. the plan is not so excellence. but It's my final plan.

guoshencheng commented 5 years ago

@Yomguithereal my PR at DefinitelyTyped has been merged.

yuchi commented 5 years ago

So, you made DefinitelyTyped accept a PR for a definition that covers this PR, while it has not yet accepted?

guoshencheng commented 5 years ago

@yuchi the PR at DefinitelyTyped has been merged. i don not know why the DefinitelyTyped merged without author 's agreement. Sorry It's my first PR for that and am i made a big trouble?

yuchi commented 5 years ago

Well, now TypeScript users will think that they have to use blessed-** in their JSX, while this is not supported at all. To avoid confusion from users I believe @Yomguithereal must now quickly react to this issue, find a solution, and (eventually) provide a patch to DefinitelyTyped.

Not a big problem IMO, but you are forcing the hand on @Yomguithereal a bit :)

yuchi commented 5 years ago

Also, I don‘t believe that plan b is a good commit message 😅

Yomguithereal commented 5 years ago

What's more I am now completely lost :). Can someone recap all this?

yuchi commented 5 years ago

Recap

  1. @guoshencheng is using react-blessed along with TypeScript.
  2. Tries to educate TypeScript about react-blessed JSX element types. This would let him have autocomplete and type check for props (among other nice features, such as autocomplete over element names.)
  3. As he correctly exposes in issue #84, he reaches a point where he finds an issue with the button component, which is both in HTML and react-blessed. TypeScript is not happy to overwrite the standard React button definition, and errors.
  4. Therefore he needs to somehow “rename” react-blessed element types. In #85 he proposes to add a mapping function to the render() entry point to have unique names, and therefore be able to add types.
  5. He is not satisfied with the solution (and I actually have to say that it was not elegant) and proposes a new one here, in #86. The new approach is simply letting users optionally prefix elements with blessed-, for example <blessed-button … />. Those will never collide with standard HTML elements, and can be perfectly typed in TypeScript.
  6. He then creates a PR on Definitely Typed (a collection of community provided TypeScript types definitions for projects that do not provide one themselves) to add all blessed-* types — expecting the reviewers to wait for the react-blessed authors for approval.
  7. Definitely Typed maintainers actually accept the PR.
  8. Now users will have types installed, but propose blessed-* elements on autocomplete, and those will not work since this PR has not been merged (and released!) yet.

I personally believe that @guoshencheng proposal here is perfectly coherent and way better than the one he suggests on #85. Since using blessed-box instead of box is optional but brings types, the choice is completely in the hand of the user (either library or app developer) and brings no compatibility issue that would arise with #85 (what if I rename button to btn but a library uses button?).

yuchi commented 5 years ago

My opinion in short: after a good commit squash (sorry I’m a git history purist!) this PR solves the issue perfectly at the cost of some more typing for users.

The only other way would be to go in the react-pdf way (relevant snippet here):

// react-blessed
export const Box = 'box';
export const Button = 'button';
// ...
// some-app.js
import React from 'react';
import { render, Box, Button } from 'react-blessed';
render(<Box … />)

This would let @guoshencheng make TypeScript think that Box etc. are not strings, but concrete React.ComponentType<Props>.

guoshencheng commented 5 years ago

thanks to @yuchi ! you make a better solution for less typing and it make the relations between tag and bless's component to be controlled by react-blessed self. I would consider to recode my merge request in one or two days.

Yomguithereal commented 5 years ago

I don't like much the

import { Box, Button } from 'react-blessed';

solution. It's hard to maintain and tedious. But typing blessed-button also is in a way. The way typescript interacts with react and/or jsx seems very precarious to me though. Anyway I would vouch for the solution of this PR. Let me just review the PR. (On a side note, there is no scheme such as the jsx pragma for TypeScript to handle those kind of cases gracefully?)

Yomguithereal commented 5 years ago

So is the PR ok for everyone involved? Can I merge and release in a near future?

guoshencheng commented 5 years ago

So is the PR ok for everyone involved? Can I merge and release in a near future?

It's ok

yuchi commented 5 years ago

Perfect :+1: (IMHO)

yuchi commented 5 years ago

But I suggest @Yomguithereal to squash all commits into a single one, or use an explicit merge commit.

Yomguithereal commented 5 years ago

v0.4.0 is now released on npm.

bensleveritt commented 5 years ago

For what it's worth, this is in part @types/react's fault. The types in there are too DOM specific (considering React is used without the DOM in many places). There is an effort to fix this, which when done will allow react-blessed types to reuse DOM element names.