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

chore(build): swap in tsdx #212

Merged
merged 20 commits into from
Oct 18, 2019
Merged

Conversation

RichiCoder1
Copy link
Contributor

There's still work that needs to happen as when I last worked on this there were still some important differences in the output around externals, but I figured I'd issue this PR so one can see the differences in a rough state.

/cc @markerikson

@markerikson
Copy link
Collaborator

Great, thanks! What are the differences in output atm?

@markerikson
Copy link
Collaborator

I'm poking around at this, and I'm seeing some failing type tests that... shouldn't be.

In particular, these for createSlice:

  // typings:expect-error
  const stringReducer: Reducer<string, PayloadAction<number>> = slice.reducer
  // typings:expect-error
  const anyActionReducer: Reducer<string, AnyAction> = slice.reducer

@phryneas , any idea why these are failing? Not sure if it's due to a TS update, or the fact that we're using TS directly now.

Also, I see that ts-jest is flagging unused variables in tests as errors, which seems overly strict (especially when a few of the tests are doing things like grabbing extraArg from a thunk and asserting on it, and not using dispatch or getState). That's... annoying.

@RichiCoder1
Copy link
Contributor Author

Great, thanks! What are the differences in output atm?

I think the biggest difference is you had externals for a few of the packages setup differently than how it's currently configured. I forget the specifics. I think there's also at least one rollup plugin I hadn't brought over yet.

I'm poking around at this, and I'm seeing some failing type tests that... shouldn't be.

In particular, these for createSlice:

Weird. Maybe a typescript bump error?

Also, I see that ts-jest is flagging unused variables in tests as errors, which seems overly strict (especially when a few of the tests are doing things like grabbing extraArg from a thunk and asserting on it, and not using dispatch or getState). That's... annoying.

That should be pretty easy to fix, just need to crank that option down.

@markerikson
Copy link
Collaborator

markerikson commented Oct 13, 2019

Yeah, looking at the build output, the biggest immediate differences I see are:

  • The names of the UMD files changed (r-s-k.umd[.min].js for current, r-s-k.umd[.development].js for this branch)
  • The UMD files are no longer embedding the other libs directly, but rather expecting them as globals

I'm not as overly concerned about the UMD naming, especially since this is pre-1.0, but I do want our UMD files to be entirely self-contained (ie, I should be able to link the UMD file and write working Redux code right there).

Also, could we rebase this against the v0.8-integration branch? I'd like to do the 0.8 release with this if we can get it pulled together.

@RichiCoder1
Copy link
Contributor Author

I think both are those are pretty fixable. Just need to drop the externals plugin from the UMD build and tweak the output name. I can try both those later.

@phryneas
Copy link
Member

phryneas commented Oct 13, 2019

I'm poking around at this, and I'm seeing some failing type tests that... shouldn't be.

Seems to be a bug introduced with TypeScript 3.6. State is inferred to any in that version, no matter what is being passed in. No idea why though. That'll need some investigation.

PS: seems that TS3.6 infers the state type to any because extraReducers is taken into account even though it is optional.
This can be fixed by changing line 69 in createReducer from

  extraReducers?: CaseReducers<State, any>
//to
  extraReducers?: CaseReducers<NoInfer<State>, any>

@markerikson
Copy link
Collaborator

Bah, closed by the TS fix PR. Reopening.

@markerikson markerikson reopened this Oct 16, 2019
@markerikson
Copy link
Collaborator

markerikson commented Oct 16, 2019

Welllll, something is definitely still not right here:

    src/createSlice.test.ts:104:42 - error TS2769: No overload matches this call.
      Overload 1 of 2, '(payload?: undefined): WithPayload<undefined, Action<"ADD_MORE">>', gave the following error.
        Argument of type '{ amount: number; }' is not assignable to parameter of type 'undefined'.
      Overload 2 of 2, '(payload?: void | undefined): WithPayload<void, Action<"ADD_MORE">>', gave the following error.
        Argument of type '{ amount: number; }' is not assignable to parameter of type 'void'.
    104       const result = reducer(10, addMore({ amount: 5 }))
                                                 ~~~~~~~~~~~~~

waitasec, that's the thing we just did in 0.8. um...

okay, we've got a createSlice test that's manually calling createAction. Why didn't that fail in the 0.8 branch in the first place? Was it because we weren't on TS 3.6 yet?

@netlify
Copy link

netlify bot commented Oct 16, 2019

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

Built with commit 0af1c4e

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

@markerikson
Copy link
Collaborator

Well, it builds now!

I fixed the now-broken uses of createAction, and patched up the types in createReducer.test.ts enough to let it run. (The point of that file is to test behavior, not the types, so I'm not worried if it's a little iffy.)

So, now the question is what the output is like.

I gotta head to bed, but will hopefully be able to look at it again tomorrow.

@markerikson markerikson mentioned this pull request Oct 16, 2019
12 tasks
- Removed "externals" so that all libs get bundled in
- Fixed dev UMD output file name
@markerikson
Copy link
Collaborator

Okay, fixed up the UMD externals and the dev filename. Output now looks basically equivalent to what we have already, minus a few Babel helpers and such.

Also, interestingly, the CJS file appears to already be minified, which is surprising.

I'm going to publish an alpha build and see if we can get some folks to test it out.

@markerikson markerikson marked this pull request as ready for review October 17, 2019 03:37
@markerikson
Copy link
Collaborator

Published v0.9.0-alpha.0 as redux-starter-kit@build-test.

Can folks try it out and see how it looks?

@phryneas
Copy link
Member

phryneas commented Oct 17, 2019

You can look at the files in the package over at https://unpkg.com/browse/redux-starter-kit@0.9.0-alpha.0/ :

I'm not really happy that we're shipping the src folder (I already know I'll be importing from there on accident), and in addition to that all those .d.ts files seperately instead of a rolled-up .d.ts (I already know that those will be problems with my babel+babel-preset-typescript webpack setup 😢 ).

@nickmccurdy can we do a rollup from that? This seems to be an open issue with jaredpalmer/tsdx#80 - that's why I personally prefer pika with @pika/plugin-bundle-types over tsdx. But maybe we could take some inspiration from jaredpalmer/tsdx#80 or how @pika/plugin-bundle-types does it?

PS: yes, it already looks like that in older versions, but I'm already doing very weird imports from RSK as I recall. Will check back on those later on work.

@phryneas
Copy link
Member

As this is a quite exotic case, I've created a reproduction repo here: https://github.com/phryneas/reproduction-non-rollup-types/blob/master/src/index.ts

Given that by now, almost everything is re-exported from index.d.ts, I don't actually think this is too much of a problem any more (it was in the early days of RSK for me and I still have some workarounds for that in place!), but I'd love to see the types as one single file for 1.0.

@phryneas
Copy link
Member

Oh, and one thing that is definitely a bug and needs to be adressed:
This file https://unpkg.com/browse/redux-starter-kit@0.9.0-alpha.0/dist/index.js links to ./redux-starter-kit.cjs.production.min.js, which does not exist.

@markerikson
Copy link
Collaborator

markerikson commented Oct 17, 2019

@phryneas : y'know, I just realized that's why we only have a minified CJS file. It's because the custom Rollup config is overriding the output in both cases, by reading from one of the package.json fields, and thus the prod build is overwriting the debug build. We'd want to change the package.json field to point to index.js instead.

I'll fix things up this evening, including applying the DTS output, and publish another alpha.

- Removed wrong filename causing CJS dev build to be overwritten
- Change package `main` field to point to CJS index file
@markerikson
Copy link
Collaborator

Been working on this this evening. Fixed the CJS filename, and was working on copying the changes @phryneas suggested.

I just updated TSDX to 0.10.x on general principle... and it looks like it's now bundling all the typedefs into index.d.ts automatically due to a change in Rollup TS plugins, no api-extractor package needed:

jaredpalmer/tsdx#200

jaredpalmer/tsdx#208

Based on that, I'm going to drop it from the setup for now. If someone wants to see what other options it offers down the road, I'm fine with that.

@markerikson
Copy link
Collaborator

Oooo-kay, and that's completely dying in the tslint check with:

Definition for rule '@typescript-eslint/no-angle-bracket-type-assertion' was not found @typescript-eslint/no-angle-bracket-type-assertion

For every TS source file. What in the world?

@markerikson
Copy link
Collaborator

Whoa, this is bizarre. That actually showed up previously over in #198 .

Prevents a whole bunch of repeats of this error:

Definition for rule '@typescript-eslint/no-angle-bracket-type-assertion'
was not found @typescript-eslint/no-angle-bracket-type-assertion
# Conflicts:
#	package-lock.json
#	package.json
@markerikson
Copy link
Collaborator

Still don't know what's causing that weird lint error, but I was able to shut it up by explicitly disabling that lint rule.

@markerikson
Copy link
Collaborator

yayyyyyy and we're green!

okay, I'm gonna go publish another alpha now.

@markerikson
Copy link
Collaborator

Okay, v0.9.0-alpha.1 is out. Unpkg looks good:

https://unpkg.com/browse/redux-starter-kit@0.9.0-alpha.1/dist/

@markerikson
Copy link
Collaborator

markerikson commented Oct 18, 2019

Tried forking the last "Advanced Tutorial" sandbox and updating it to v0.9.0-alpha.1. Fixed the slice -> name changes, and it runs.

Any further comments or objections before I merge this in and publish 0.9.0?

Or, should I just go ahead and jump to 1.0.0-rc.0?

@phryneas
Copy link
Member

I'd publish it as 0.9 - that way we get some real-world feedback until you publish it as 1.0

@markerikson
Copy link
Collaborator

Agreed. All right, I'll merge this in and put out 0.9.

@markerikson markerikson merged commit d639996 into reduxjs:master Oct 18, 2019
@markerikson
Copy link
Collaborator

@RichiCoder1 RichiCoder1 deleted the switch_to_tsdx branch October 20, 2019 22:22
@RichiCoder1
Copy link
Contributor Author

@markerikson Glad I could be of help!

@markerikson
Copy link
Collaborator

Yep, thanks!

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.

4 participants