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

Update to Babel 7 #71

Closed
wants to merge 2 commits into from
Closed

Update to Babel 7 #71

wants to merge 2 commits into from

Conversation

denisw
Copy link
Contributor

@denisw denisw commented Jan 7, 2019

This is a pre-requisite to using @babel/preset-typescript later.

This change required upgrading rollup-plugin-babel, which in turn required upgrading Rollup.js itself. Fortunately, the upgraded rollup-plugin-babel automatically takes care of Babel helpers deduplication and turning off module transpilation in Babel's env preset, so I was able to drastically simplify the Babel configuration.

For the same reason, we don't need rollup-plugin-replace anymore.

@netlify
Copy link

netlify bot commented Jan 7, 2019

Deploy preview for redux-starter-kit-docs ready!

Built with commit 9432c6b

https://deploy-preview-71--redux-starter-kit-docs.netlify.com

@denisw denisw mentioned this pull request Jan 7, 2019
@Dudeonyx
Copy link
Contributor

Dudeonyx commented Jan 7, 2019

Hi, I'm the guy who offered to help convert rsk to typescript and I'd like to work together with you.
I've already created a typescript version of it in https://github.com/Dudeonyx/redux-ts-starter-kit, with some other internal changes, especially in createSlice(), most of the changes were merged into the original robodux last night.

@denisw
Copy link
Contributor Author

denisw commented Jan 7, 2019

@Dudeonyx Hey, that's good to hear! I started a WIP TypeScript version based on this PR and #70, but I'm sure your version has a lot of stuff already figured out that we can incorporate. I'll create a work-in-progress PR in a few hours (at work right now) and then we can take it from there.

I see that your fork contains some changes on top of the conversion to TypeScript. I think the best way to go forward here would be to do a "straight" conversion without API or behavior changes first, and then look at the rest (such as the changed return value of configureStore or the selectorator-reselect swap) in separate PR's.

@Dudeonyx
Copy link
Contributor

Dudeonyx commented Jan 7, 2019

I see that your fork contains some changes on top of the conversion to TypeScript. I think the best way to go forward here would be to do a "straight" conversion without API or behavior changes first, and then look at the rest (such as the changed return value of configureStore or the selectorator-reselect swap) in separate PR's.

I was thinking the same, let's do the conversion first and in subsequent PR's we can discuss the benefits (or lack of) then.

But when we're done, I strongly believe we should move away from selectorator though because from a practical stand point it isn't type safe, the returned selectors always have a generic Function type. No indication of what input it accepts or what output it gives. At that point you might as well use any.

Due to the fact that selectorator accepts strings as input, This won't ever be remedied unless Typescript as a whole is overhauled.

Scratch that, we can simply alter the typing so the user can specify an input & output type when creating the selector, along the lines

import { createSelector } from "redux-starter-kit";

 interface State {
  foo: {
  bar: string;
  };
  baz: string;
 }
                            // State is input type, string is output type
const getBarBaz = createSelector<State, string>(["foo.bar", "baz"], (bar, baz) => {
  return `${bar} ${baz}`;
});

// getBarBaz() has signature: (state: State) => string;

const state = {
  foo: {
    bar: "bar"
  },
  baz: "baz"
};

console.log(getBarBaz(state)); // "bar baz"

in fact I'm gonna head over to the selectorator repo and make the PR

@Dudeonyx
Copy link
Contributor

Dudeonyx commented Jan 7, 2019

made the PR planttheidea/selectorator#13

Early signs from the maintainer are positive so If it gets merged we can use selectorator else use a custom fork of it

@denisw denisw mentioned this pull request Jan 7, 2019
9 tasks
@markerikson
Copy link
Collaborator

markerikson commented Jan 11, 2019

So I just tried checking out this branch, merged master over to pick up a couple tweaks to the UMD output, did a build, and compared the build output from master and this branch.

The output is similar, but all of the Babel 7 files actually wind up bigger by some amount. Inspecting the actual output files, it's a variety of small changes. I'm not sure how much of that is changes in Babel 7, and what might be a change in the Babel config. For example, string concatenations went from camelize('get ' + slice) to camelize("get ".concat(slice))

edit

Although I guess I should note that that's looking at the unminified build output in dist. Not sure how much of a difference that makes in prod.

@denisw
Copy link
Contributor Author

denisw commented Jan 11, 2019

For example, string concatenations went from camelize('get ' + slice) to camelize("get ".concat(slice))

This is apparently a change made for spec compliance. The docs for @babel/plugin-transform-template-literals explain this:

When [the loose option is] false or not set, all template literal expressions and quasis are combined with String.prototype.concat. It will handle cases with Symbol.toPrimitive correctly and throw correctly if template literal expression is a Symbol(). See babel/babel#5791.

This is a pre-requisite to using @babel/preset-typescript later.

This change required upgrading rollup-plugin-babel, which in turn
required upgrading Rollup.js itself. Fortunately, the upgraded
rollup-plugin-babel automatically takes care of Babel helpers
deduplication and turning off module transpilation in Babel's env
preset, so the Babel configuration could be drastically simplified.
For the same reason, we don't need rollup-plugin-replace anymore.
@markerikson
Copy link
Collaborator

Superseded by #73 .

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.

None yet

3 participants