osdevisnot commented 5 years ago

What this PR does

[x] moves minify from now unmaintained uglify to terser, closes #150
[x] sets foundation for typescript migration as mentioned in #134
[x] adds prettier support for consistent formatting
[x] use more ES6/Typescript features
- [x] arrow functions
- [x] const & let

Future Work

[ ] switch to typescript strict mode
[ ] Use more ES6/Typescript features (Map, destructuring, etc.)

Todo before merging this PR

[ ] match typings for public functions
[ ] squash all commits into one (or at least fewer) to reduce noise.

Please note, this initial PR attempts to achieve above features with least possible changes to src. Main objective for this PR is to collect feedback from community.

Thoughts / Opinions ?

codecov-io commented 5 years ago

Codecov Report

Merging #155 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #155   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         183    189    +6     
  Branches       52     63   +11     
=====================================
+ Hits          183    189    +6

Impacted Files	Coverage Δ
src/superfine.ts	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4691379...c26889a. Read the comment docs.

mindplay-dk commented 5 years ago

Looks like you lost the references to the original .js source-files when you renamed to .ts?

You should use git mv when changing the name of an existing file, otherwise we lose the source history of the files entirely.

Looks promising - though it's a bit hard to review changes without a diff :-)

osdevisnot commented 5 years ago

@mindplay-dk Thank you for checking. So far I tried using git mv (see first commit in this PR) then apply change. I also tried a fresh commit with all changes applied and files moved using git mv (see develop branch on my fork).

However, nothing seems to provide a diff in github. The history on individual files, is now correctly preserved (with git mv), for eg, here is what my webstorm tells me for history of main file:

I will try more tomorrow to get github show correct diff, but their algorithm appears to have few hardcoded limits (see diffcore-renam.c

Funny enough, when I navigate individual commits, I am able to see correct diff. Check this for example, https://github.com/jorgebucaran/superfine/pull/155/commits/88e44876be9c368795ffda6ffa7c15b57e901601

mindplay-dk commented 5 years ago

Funny enough, when I navigate individual commits, I am able to see correct diff.

Yeah, huh. Not your bad then. Probably should file a bug report with GitHub?

Now that I can see the diff - I see you've added semi-colons to the entire codebase; I don't personally have any strong feelings one way or the other, but that's a style-change, unrelated to the changes you're proposing, and it's making the diff extremely noisy and hard to read.

A code style change probably belongs in a separate proposal. Which I'm fairly sure @jorgebucaran would reject 😉

osdevisnot commented 5 years ago

I somehow overlooked code style changes. Changed from semi to no-semi and from single quote to double quote in my latest commits.

Also, it seems github magically started showing the PR diff as well :

osdevisnot commented 5 years ago

Also, this is how generated d.ts file looks so far

declare global {
    namespace JSX {
        interface Element extends VNode<any> {
        }
        interface IntrinsicElements {
            [elemName: string]: any;
        }
    }
}
export interface VNode<Props = {}> {
    name: string;
    props: Props;
    children: Array<VNode>;
    element: Element | null;
    key: string | null;
    type: number;
}
export declare type Children = VNode | string | number | null;
export declare const recycle: (container: any) => {
    name: any;
    props: any;
    children: any;
    element: any;
    key: any;
    type: any;
};
/**
 * Render a virtual DOM node into a DOM element container.
 * @param lastNode The last virtual DOM node.
 * @param nextNode The next virtual DOM node.
 * @param container A DOM element where the new virtual DOM will be rendered.
 */
export declare const patch: (lastNode: VNode<{}>, nextNode: VNode<{}>, container: Element) => VNode<{}>;
export declare const h: (name: any, props?: any, ...rest: any[]) => any;

Next up, figure how to correctly type h (current approach causes error when we try to access children in h)

mindplay-dk commented 5 years ago

Also, it seems github magically started showing the PR diff as well

Likely because the files are now similar enough to look like the same file :-)

mindplay-dk commented 5 years ago

Lots of work to do in terms of hinting params and return-types, but this looks like a good start towards the goal of using more ES6 features and porting to Typescript :-)

If I may, I'd like to suggest merging this to a separate branch (e.g. typescript) so that further work can PR against the working branch, so we can incrementally review further changes, until it's finally ready to PR the complete work against master? Just a thought.

jorgebucaran commented 5 years ago

@osdevisnot I agree, migrating to terser and migrating to TypeScript are different things. Could this PR only take care of the first?

mindplay-dk commented 5 years ago

Yeah, the switch to terser makes sense now - while the TypeScript switch is probably a much longer process.

A separate PR for the switch to terser would make a lot of sense.

osdevisnot commented 5 years ago

@mindplay-dk @jorgebucaran makes sense. I will close this PR and create 2 separate ones for separation of concerns.

jorgebucaran / superfine

migrate to terser, sets foundation for typescript migration #155

Codecov Report