Skip to content
This repository has been archived by the owner on Jun 8, 2021. It is now read-only.

extra /react entrypoint #141

Merged
merged 4 commits into from
Jan 24, 2021
Merged

extra /react entrypoint #141

merged 4 commits into from
Jan 24, 2021

Conversation

phryneas
Copy link
Collaborator

@phryneas phryneas commented Jan 1, 2021

let's see how this works out

Closes #141

@github-actions
Copy link

github-actions bot commented Jan 1, 2021

size-limit report 📦

Path Size
ESM full 9.78 KB (-10.21% 🔽)
createBaseApi + setupListeners 0 B (-100% 🔽)
createApi + setupListeners 8.93 KB (-9.64% 🔽)
fetchBaseQuery 632 B (-0.16% 🔽)
retry 271 B (0%)
ApiProvider 400 B (0%)
CJS minfied 15.06 KB (-8.22% 🔽)
ESM full (React) 10.86 KB (+100% 🔺)
createApi (React) + setupListeners 9.89 KB (+100% 🔺)
CJS React minfied 16.4 KB (+100% 🔺)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 1, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5946282:

Sandbox Source
React Configuration
React Typescript Configuration
rtk-query-demo Configuration
svelte-app-rtk-simplequery-demo Configuration

@phryneas phryneas force-pushed the react-entrypoint branch 5 times, most recently from 3db9546 to 770c182 Compare January 1, 2021 21:10
@phryneas
Copy link
Collaborator Author

phryneas commented Jan 1, 2021

So, I think this should cover everything.

@Andarist, I would be very grateful if you could take a look at this if I did anything horribly wrong :)

@@ -5,10 +5,11 @@ import replace from '@rollup/plugin-replace';

/** @type {import("rollup").RollupOptions} */
const defaultConfig = {
input: 'src/index.ts',
input: ['src/index.ts', 'src/react.ts'],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: any special reason why you'd like to keep using preserveModules?

Copy link
Collaborator Author

@phryneas phryneas Jan 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compared to a single-file ESM build, tree shaking seems to be working a LOT better.

With preserveModules (0.2.0, built with this config):
image
Normal rollup build (0.1.0, built with tsdx):
image

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was expecting this answer and it's a good one. Just a note for completeness - this doesn't quite improve tree-shaking (given my own definition of what tree-shaking is). It allows bundlers to leverage "sideEffects": false - but this flag is a hack around tree-shaking as ideal tree-shaking in practice is sometimes hard to achieve (due to initialization side-effects that include function calls and property accesses).

Both things are not mutually exclusive though as "sideEffects": false relies heavily on the file boundaries that you have set up as an author which might not produce 100% ideal results either.

@Andarist
Copy link

Andarist commented Jan 2, 2021

@phryneas nothing obviously broken here 👍 it's a lot of delicate configs so I can't give any guarantees that this works, but I know you have done some manual testing already, so 🤞

@phryneas
Copy link
Collaborator Author

phryneas commented Jan 2, 2021

@Andarist one bonus question: do you know how to keep it from stripping // @ts-ignore comments?
The example from #138 still type-checks even non-included files & errors as soon as it encounters the react-redux type import. /* @ts-ignore */ is being preserved through build, but TS does not accept that.

@msutkowski msutkowski linked an issue Jan 3, 2021 that may be closed by this pull request
Copy link
Member

@msutkowski msutkowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. We'll need to update docs as well before a release. I only have one real question - do the types need to be moved from /ts/ -> /esm/ts for a particular reason?

I'm wondering if this should also be documented in release notes, as it could be a breaking change for some folks?

examples/svelte/src/services/counter.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@phryneas
Copy link
Collaborator Author

phryneas commented Jan 3, 2021

This looks good to me. We'll need to update docs as well before a release. I only have one real question - do the types need to be moved from /ts/ -> /esm/ts for a particular reason?

Simplicity of the build. We'd need an additional step otherwise, as only the ESM build builds all entry points, but TS wants to emit only to sub-folders of the ESM build.

I'm wondering if this should also be documented in release notes, as it could be a breaking change for some folks?

Hmm. It's not really "documented API" in my mind. Only what is exported from index (or other "official entry points") has a guarantee to stay the same between versions, and that entry point still exports the same types as before (minus the react ones).

@msutkowski
Copy link
Member

This looks good to me. We'll need to update docs as well before a release. I only have one real question - do the types need to be moved from /ts/ -> /esm/ts for a particular reason?

Simplicity of the build. We'd need an additional step otherwise, as only the ESM build builds all entry points, but TS wants to emit only to sub-folders of the ESM build.

I'm wondering if this should also be documented in release notes, as it could be a breaking change for some folks?

Hmm. It's not really "documented API" in my mind. Only what is exported from index (or other "official entry points") has a guarantee to stay the same between versions, and that entry point still exports the same types as before (minus the react ones).

Makes sense to me, thanks!

@phryneas phryneas mentioned this pull request Jan 5, 2021
@msutkowski msutkowski linked an issue Jan 22, 2021 that may be closed by this pull request
@phryneas
Copy link
Collaborator Author

Umh, yeah, forgot about this. LGTM.

@phryneas phryneas merged commit 3c8adf0 into next Jan 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rtk-query requires react type definitions (not framework-agnostic) Investigate size optimization
3 participants