chore: native typescript remove all eslint annotation and rollback version #2285
Conversation
|
@2heal1 The rpc section can add some tests |
|
@ilteoood I don't know where exactly you're referring to readme，Can you be more specific? |
Hi, I'm referring to the readme that's inside the plugin directory: https://github.com/module-federation/universe/blob/main/packages/native-federation-typescript/README.md As you can see, it includes an explaination of the functionalities, the configuration structure and how to use it for every supported browser. If something changed under the hood (e.g. additional configs or new functionalities) they should be also documented here. Since this is also published to NPM, it should be always updated as well to allow users to have a clear documentation while they download the package |
@2heal1 I think you can help to supplement the new configuration items in this part, and add more instructions |
Yeah sure, i will make n new pr to supplement README |
✅ Deploy Preview for module-federation-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for module-federation-new ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
2heal1
force-pushed
the
chore/optimize-federation-type-code-style
branch
from
9200edf
to
5eab747
Compare
|
@ilteoood The problem has been fixed, you can help check again |
zhoushaw
changed the title
There was a problem hiding this comment.
I have looked at this PR together with the code that has been already merged, and I would like to make the following considerations:
- Some of the configuration parameters have been introduced even if not needed: IMO we should consider removing them and force the user to remove the plugin from the chain instead of soft-skip it in order to improve performances;
- Some of these new configurations have not been declared: e.g.
Property 'consumeAPITypes' does not exist on type 'Required<HostOptions>'. - Some of these configurations have been wrongly implemented: e.g., if
consumeAPITypesis configured to false, we are still loggingFederated types extraction completed. If the user is skipping it, it should not appear; - The
NativeFederationTypeScriptHostbehaviour is completely broken at the moment: theconsumeTypesPromiseis created just once, so the types are synced only at the bundler startup. In all the other cases, we are serving an already fulfilled promise which doesn't do anything.
Said so, since me, my company (which sponsored the plugins) and other people I involved during meetups rely a lot on the correct behaviour of these plugins, may I ask you to bring back the original implementation?
I would love to help you achieving what you need, but I really think this is not the correct way to do it.
As an alternative, since I understand your will of being completely indipendent from me, I suggest you to copy what you need into another plugin and proceed in the way you like
| @@ -45,6 +50,7 @@ This plugin is used to download the federated types. | |||
| typesFolder?: string; // folder where all the files will be stored, default is '@mf-types', | |||
| deleteTypesFolder?: boolean; // indicate if the types folder will be deleted before the job starts, default is 'true' | |||
| maxRetries?: number; // The number of times the plugin will try to download the types before failing, default is 3 | |||
| consumeAPITypes?: boolean; // whether to generate the type of runtime `loadRemote` API | |||
There was a problem hiding this comment.
NativeFederationTypeScriptHost just download stuff, it doesn't generate any type at runtime.
Additionally, do we really need an option for this? If the user doesn't want to consume the types it would be better to exclude the plugin from the chain, to also improve performances and avoid false positive messages.
I'm already doing that in production
| @@ -25,6 +25,11 @@ This plugin is used to build the federated types. | |||
| deleteTypesFolder?: boolean; // indicate if the types folder will be deleted when the job completes, default is 'true' | |||
| additionalFilesToCompile?: string[] // The path of each additional file which should be emitted | |||
| compilerInstance?: 'tsc' | 'vue-tsc' // The compiler to use to emit files, default is 'tsc' | |||
| generateAPITypes?: boolean; // whether to generate the `loadRemote` type in `Federation Runtime` | |||
There was a problem hiding this comment.
Do we need this?
If the user doesn't want to generate types, he should exclude the plugin for the chain.
@ilteoood I understand what you mean , so can we copy the currently implementation to another package ? And we will revert the |
|
Because we want to combine the types with runtime as well , and it aslo deeply coulpe with bundler , so it may not fit the Split the types into two packages should be better solutions, we will note the sources of key implementations in our new package. |
|
Even if I still think there's no need to couple this kind of stuff to the bundler, because typescript itself is bundler-agnostic, I prefer to have the same code copied into two different places rather than a broken plugin. |
|
Thanks . I agree that typescript itself is bundler-agnostic, but we need to record it to manifest and add to assets to suit our needs . I will try to make a clearer distinction between the types generatation and bundler . Back to the topic , i will revert your prev implementation in this pr . |
|
I would be interested in discuss it further, but let's go in this way for the moment |
|
@ilteoood I have reverted the previous implementation , and add changest to bump version to help correct the latest tag. please help to check , if no problems , approve pls. |
Description
Related Issue
Types of changes
Checklist