Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

migrate to terser, sets foundation for typescript migration #155

Closed
wants to merge 0 commits into from
Closed

migrate to terser, sets foundation for typescript migration #155

wants to merge 0 commits into from

Conversation

osdevisnot
Copy link
Contributor

@osdevisnot osdevisnot commented Dec 5, 2018

What this PR does

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
Copy link

codecov-io commented Dec 5, 2018

Codecov Report

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

Impacted file tree graph

@@          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
Copy link
Contributor

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
Copy link
Contributor Author

osdevisnot commented Dec 6, 2018

@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:

image

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, 88e4487

@mindplay-dk
Copy link
Contributor

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
Copy link
Contributor Author

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 :
image

@osdevisnot
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Owner

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

@mindplay-dk
Copy link
Contributor

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
Copy link
Contributor Author

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

@osdevisnot osdevisnot closed this Dec 20, 2018
@osdevisnot osdevisnot mentioned this pull request Dec 21, 2018
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using Terser to minify
4 participants