yysun/apprun

Building in strict mode

phBalance opened this issue · 12 comments

phBalance commented

Hi,
while not a bug, it would be nice if we could get AppRun built in strict mode. As @yysun has indicated in #71, it's a goal. To that end I started taking a look, but thought that I'd throw a few things out for comments:

  1. Improve the creation of apprun.d.ts: Right now, since it's not auto generated from the code, there are situations where the main type file is out of sync with the code. I would suggest that we need to make this file as auto generated as possible. One possible move in this direction would be to move lots of the types into the src/types.ts file that exist right now. We could then convert this into a declaration file and use it to build the src (and then copy over during the build process). Thoughts?

  2. deprecated exports in apprun.d.ts: In the vain of cleaning up the API, there is at least a comment in this file indicating an obsolete API (update). If this is actually the case, we ought to make an effort to deprecate it given the change was made ~2 years ago.

  3. conflicting types: Right now I see that there is an Element type defined - which conflicts with the DOM's definition. We could namespace the types and play well with others or we could change the name to AppRunElement or something of the sort. Preferences?

    There is still lots of combing through the code to clean things up but those seem to me to be some first questions to answer and most of the rest ought to be mechanical.

Cheers,
Peter

yysun commented
phBalance commented

Hi @yysun,
I'd not actually had a look at the V2 branch to date. But looking at it now, it seems like the only real differences between it and the V1 branch are:

  1. A few build changes (e.g. using rollup and lint)
  2. Adding web components, which are equally usable in es5 as they're, unless I'm mistaken, an HTML5 things.

Unless I'm missing something, there's nothing in v2 which couldn't be in 1 from an ecma point of view. If so, let's create v3 from v2 as the starting point or merge all of v2 into v1 and build v3 at a later date.

I'll let you create the branch as you wish.

Cheers,
Peter

yysun commented

Hi @phBalance,

I have pushed the V3 branch. I am looking for a way to maintain compatibility:

npm install apprun - for the es5 build
npm install apprun@es6 - for the es6 and esm builds

phBalance commented

Hi @yysun,
I'm plugging away at the strict stuff. Perhaps you can shed some light on what the correct behaviour is with regards to component's options.history?

Right now:

  • component.start(..., options: { history? } ).

    • The history property doesn't have a type associated with it.
    • The one test case using it associates a boolean type i.e. {history: true}
    • The value of history is passed through to this.mount's options.

  • component.mount(..., options: { history? } )

    • calls this.on(options.history.prev, etc)
    • the first parameter of component.on is event: E. which indicates that it's type of history should be {prev: E, next: E}
    • it's not clear if prev and next can be omitted as there is a check (e.g. options.history.next || 'history-next')

    There are problems with this as it is:

  1. The value of history from component.start is being passed through and is not the same type (boolean vs {prev: E, next: E} or {prev?: E, next?: E}).

  2. Is the type history: {prev: E, next: E} or history: {prev?: E, next?: E}?

  3. If prev and next are optional, component.mount's call to this.on is a problem as the default value of 'history-prev' or 'history-next' are strings and not type E. Presently E has no constrains (like it needs to extend string) so we can't use a string type.

So, my questions are:

  1. What is the type of options.history in component.start? Should it be the same as component.mount's options.history?

  2. Should there be constraints on Component<T,E>'s generic type E?

  3. If there are no constrains on E's type, what is the appropriate default value for history or should there be none?

    I can modify the test cases as required.

Thanks,
Peter

yysun commented

component.start calls component.mount by setting the option {render:true}. The options.history is passed to mount without change., so options.history is the same in start and mount.

History option should look like:

type HistoryOption = boolean | {prev?: string, next?: string};

The default values are history-prev and history-next. They are event names. See https://jsfiddle.net/ap1kgyeb/6

In terms of the event type E in Component<T, E>, the idea is to use the Discriminated Union, which is explained here: https://medium.com/@yiyisun/strong-typing-in-apprun-78520be329c1

type Events = '-1' | '+1';
const update: Update<number, Events> = [ ... ];
class Counter extends Comonent<number, Events> { ... }
phBalance commented

Hi @yysun,
thanks for the answer. If the goal is to have the Events type be a string literal, union of string literals, or a string it's easy enough to constrain the generic on Component and friends to <E extends string = string>. That'll stop E from being a number or something weirder and will default to a string rather than any.

Is the declaration of the Component expected to include 'history-prev' and 'history-event' in its E type if the Component will use history? Regardless, I'll have to add it to the permitted union of events for component.on since the typing isn't dynamic (component.on(event: E | 'history-prev' | 'history-next', ...etc).

If the history option supports boolean, that means that this little chunk of code is wrong since options.history.prev doesn't exist when it's a boolean:

apprun/src/component.ts

Lines 153 to 158 in 1013f9b

this.enable_history = !!options.history;
if (this.enable_history) {
this.on(options.history.prev || 'history-prev', this._history_prev);
this.on(options.history.next || 'history-next', this._history_next);
}

What's the desired behaviour. This?

 if (this.enable_history) {
      this.on(typeof options.history !== 'boolean' && options.history.prev ? options.history.prev : 'history-prev', this._history_prev);
      this.on(typeof options.history !== 'boolean' && options.history.next ? options.history.prev : 'history-next', this._history_next);
 }
yysun commented

We can set the history option by using { history: true } or { history: { prev: ‘prev’, next: ‘next’ }}. Either way !!options.history returns true. Otherwise false. You can see test cases in component.spec.tsx.

phBalance commented

Oh, of course. Brain fart. Trying to access a property on a boolean, while silly, is perfectly allowed in JavaScript just not in strict TypeScript.

phBalance commented

Hi @yysun,
how do you want to handle all the places where component.state is being accessed? It's a problem since it's a protected class member.

Is this something that you wanted to be readonly with a public get method?

Also, did you intend to have webComponent be generic since it has a component?

Thanks,
Peter

yysun commented
phBalance commented

Hi @yysun,
just need to indicate that it needs to provide an intersection type of the constructor and whatever public methods the component needs to have. Since it's essentially a component, which itself is generic, my thinking was that it would make sense to have it be generic as well.

yysun commented