-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Javascript Flow typed #8101
base: master
Are you sure you want to change the base?
Javascript Flow typed #8101
Conversation
This is such an awesome accomplishment! I can't wait to try it with a react app with flow. Base on the code change, could you confirm that it is this a different flavour of JavaScript client than the original ES 6 JavaScript client with
|
This generator creates Flow typed JavaScript client that utilizes [Fetch API](https://fetch.spec.whatwg.org/). The generated Node module can be used in the following environments: | ||
|
||
Environment | ||
* Node.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could anyone help provide a guideline for how to get Node.js to work with this flavour of JavaScript client? At the time of writing, Node.js doesn't support ES6 module system yet. Is there a codegen configuration we could use for the node.js environment? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just run npm install && NODE_ENV=development node run build
to transpile into valid es5 code.
however, you can use this generated code direct in your flow-enabled ES6 project because you would have transpiling in place already.
I have no experience with Flow, so I won't be able to review. |
@joyfulelement this client is different from the es6 client. simply adding flow annotations didn't work with the current code. steps to use this client as a npm library:
if you want to generate the api directly inside your client-app you can do so. just import the files from the generated |
Hi @wing328, this PR introduces a new javascript language dialect, please advise how to proceed with reviewing this changes. |
@jaypea I'm using your branch for my react-native project and it works great! 👍 Two small things I had to tweak using a custom template that you might want to take into consideration:
|
@wouterhund thanks for the feedback. to change the basePath during runtime, I feed it into the Configuration Object:
I've no experience with react native, though. |
* @export | ||
*/ | ||
export const {{classname}} = function(configuration?: Configuration, fetch: FetchAPI = portableFetch): {{classname}}Type { | ||
const basePath: string = (configuration && configuration.basePath) || BASE_PATH; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaypea Unless I'm missing something this line makes it so the basePath
cannot be changed during runtime (I don't mean just setting it once at the start of runtime). The basePath
used by the api object does not change when the basePath
stored in the configuration object changes.
My fix was to simply drop this line, and replace the reference to const basePath
with (configuration && configuration.basePath) || BASE_PATH
in the code below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wouterhund thanks for clarification. I see your point now.
I like to have my code as side-effect free as possible and like to treat the objects as immutable as possible. So changing something in Configuration
should result in a new Configuration object and be passed explicitly to the users of this object.
Ask yourself, how would you unit test your code?
I'd rather rewrap the api whenever the configuration needs to be changed.
my code looks more like that:
// apiWrapper.js
export function wrapApi() {
const myConfig = new apiclient.Configuration({
apiKey: token,
basePath: (process.env.REACT_APP_API_BASEPATH: any),
});
return { myApi : apiclient.MyApi(myConfig) }
}
// someSaga.js
import { wrapApi } from 'apiWrapper.js';
wrapApi().myApi.aMethodInside(parameter);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configuration object itself is not immutable, so I wouldn't expect it to be treated as such. The username/password (when using basic auth) are not treated as immutable either, only the basePath. If nothing else that is a confusing discrepancy.
With your example, every single time I want to invoke the api a new configuration and api object gets instantiated as well, which seems like unnecessary overhead. The function could of course cache the myApi object until it changes, but at that point I'd rather keep using my custom templates which don't suffer from this issue. This looks much more practical to me:
// api.js
const configuration = new apiclient.Configuration();
let updateCredentials = () => {
let state = store.getState().api;
configuration.basePath = state.url;
configuration.username = state.user;
configuration.password = state.pass;
}
store.subscribe(updateCredentials)
updateCredentials()
export const api = apiclient.MyApi(configuration);
// someSaga.js
import { api } from 'api.js';
api.aMethodInside(parameter);
Additionally, the regular javascript code generated by swagger-codegen also doesn't try to be immutable. And since I'm migrating from that this was a pain point.
whether/when this change going to be merged and released? this is very desired feature - imo |
When will this PR be merged? I would really like to try it out to generated a flow client to use it in Frontend. |
@ivanantipin @MichaelSu1983 please see the linked PR from May 29 in another repository to try it ;) |
any update? |
Hi , I am seeing the following error: Caused by: java.lang.ClassNotFoundException: javascript-flowtyped somebody help? |
As discussed in #5733 there is no generator which outputs Javascript which is properly flow typed and easy to use with React.
Here is it now.
Is is based on the Typescript Generator, so differs from the other Javascript Generators.
Please have a look @CodeNinjai @frol @cliffano