Register returns any, breaking typescript's type check
demedos opened this issue ยท 10 comments
Describe the bug
The register
function returns any
, breaking typescript's type check.
Code snippet
// MyComponent/index.ts
import { register } from 'react-native-bundle-splitter';
export default register({ loader: () => import('./MyComponent') });
// MyComponent/MyComponent.tsx
import React from 'react';
import { View } from 'react-native';
type Props = {
isActive: boolean;
};
export default (_props: Props) => {
return <View />;
};
// Screens/index.tsx
import React from 'react';
import MyComponent from '../MyComponent';
export default () => {
// The following code should fail the type check
return <MyComponent wrongProp1={[]} wrongProp2={false} />;
};
Repo for reproducing
https://github.com/demedos/bundle-split-repro
To Reproduce
Steps to reproduce the behavior:
- Add the bundle split to a component
- Import the component into another one
- Pass the wrong props
Expected behavior
Typescript's type check (yarn tsc
) should throw an error
Screenshots
If applicable, add screenshots to help explain your problem.
Smartphone (please complete the following information):
- Desktop OS: NA
- Device: NA
- OS: NA
- JS engine: NA
- Library version: 2.1.0
Additional context
Add any other context about the problem here.
Hi @demedos
Thank you for raising the issue. I think I've been trying to specify correct types, though it wasn't easy
As far as I remember we can not inherit type from import
function (correct me if I'm wrong). The other possible way would be making register
function to accept generic T
. But I suppose it could bring more issues because you would need to define type Props
at the same level with register
function, which is not good from code perspective (or store in separate file? It's better but far from ideal variant). I think that was the reason why I made it any
(I know it's very bad
Do you have any ideas how to fix it? Or could you at least share your thoughts?
As far as I know there's not way to infer the type from the import function.
Something like this would solve the problem, but this would also load the file immediately, making the bundle split useless.
Also, it's just a type cast, which is not ideal as we are forcing the return type. Its usage would be like so:
// MyComponent/index.ts
import { register } from 'react-native-bundle-splitter';
import MyComponent from './MyComponent';
export default register<typeof MyComponent>({ loader: () => import('./MyComponent') });
We should definitely find a way to remove the any
return type from the optimized
function and let typescript infer it
@demedos don't you think, that the line
import MyComponent from './MyComponent';
will break a concept of lazy loading? You will have a direct reference to the file and it will be bundled with main part of the application.
The idea, that you suggest I described here:
The other possible way would be making register function to accept generic T. But I suppose it could bring more issues because you would need to define type Props at the same level with register function, which is not good from code perspective (or store in separate file? It's better but far from ideal variant). I think that was the reason why I made it any (I know it's very bad
๐ )
I can add generic with default value to the code, but you as a developer will need to keep in mind, that you can not have direct references to file, otherwise it will break a concept of lazy initialization. Though you can always create file types.ts
and put following content:
// types.ts
export type Props = {
isActive: boolean;
};
And use it in next way:
// MyComponent/index.ts
import { FC } from "react";
import { register } from 'react-native-bundle-splitter';
import { Props } from './types';
export default register<FC<Props>>({ loader: () => import('./MyComponent') });
Will it work for you or not? Though, again, you need to be careful and always keep an eye on the content inside of packager
.
Also I'm going to check similar packages for web and how they bring a support for TS.
Yes, it would break the lazy loading, that's what I was saying
[...] this would also load the file immediately, making the bundle split useless.
I see no other solution than to use a generic and manually move all the props in their own files.
The developer should then check if it is necessary to use the FC interface over ReactComponent
this would also load the file immediately, making the bundle split useless.
@demedos I don't know how I missed it
Yes, it sounds like a potential option to add it. I think I will investigate it a little bit more, and if I don't find any better solutions, then I will implement these changes and will publish a new release!
I hope within several days it should be done
Sounds good, thank you
The only downside of this is that passing FC<Props>
as a generic may be wrong, because even if the source component is a FC, optimized
always returns a new pure component. Nonetheless, it should work without any problem
Yes, it is indeed a great improvement.
I use react-redux's ConnectedProps
type to infer the action and properties injected in the component through the connect
function; this means that when moving the Props
type to its own file, the connector must imported from the component file itself. Do you think that this can affect the bundle split?
See an example below:
// MyComponent.tsx
import { connect, ConnectedProps } from 'react-redux';
export const connector = connect(mapStateToProps, mapDispatchToProps);
// types.ts
import { connector } from './MyComponent';
export type Props = ConnectedProps<typeof connector>
// index.ts
import { Props } from './types';
export default register<Props>({ loader: () => import('./MyComponent') });
@demedos I bet on "yes", it will affect and the file will not be lazily loaded. Though, you can try to verify it by calling investigate
method. It returns all files, which you will have in your main bundle (these files will be loaded at the start of the application).
We also are using redux in combination with this library. But almost all the time all injected Props
by redux
we describe manually, so it wasn't an issue for us.
Closed, because #34 has been merged.