yuriipalam commented on code in PR #264: URL: https://github.com/apache/ozone-site/pull/264#discussion_r2732191889
########## src/components/SwaggerUI/index.js: ########## @@ -0,0 +1,83 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React, { useState, useEffect } from 'react'; +import SwaggerUI from 'swagger-ui-react'; +import 'swagger-ui-react/swagger-ui.css'; +import useBaseUrl from '@docusaurus/useBaseUrl'; +import styles from './styles.module.css'; + +export default function SwaggerUIComponent({ spec, defaultServer }) { + const specUrl = useBaseUrl(spec); + const [serverUrl, setServerUrl] = useState(defaultServer || 'http://localhost:9888'); + const [specData, setSpecData] = useState(null); + + useEffect(() => { + // Fetch the spec and modify the servers array + fetch(specUrl) + .then(response => response.text()) + .then(text => { + // Parse YAML to JSON (simple approach for this case) + // Since swagger-ui-react can handle YAML, we'll pass the URL + // but configure servers via the spec modification + return fetch(specUrl).then(r => r.json().catch(() => { + // If JSON parsing fails, it's YAML, use the URL directly + return null; + })); + }) + .catch(() => null); + }, [specUrl]); Review Comment: We're fetching here twice and do not store the fetched data anywhere. You initialized the state `specData`, but `setSpecData` is never called. Instead, we should call the `fetch` function once, store the result of `response.text()` and either parse it as JSON or YAML. Something like this: ```ts useEffect(() => { const fetchData = async () => { if (specUrl) { try { const resp = await fetch(specUrl); const body = await resp.text(); // assume we fetched JSON setSpecData(JSON.parse(body)); } catch (e) { // handle error return null; } } } fetchData(); }, [specUrl, setSpecData]); ``` It's also possible to do with the promise chain (.then, .catch), the output is the same in any case. Also, ideally we should add `setSpecData` to the dependancies list. All the used reactive variables should be in the useEffect, otherwise it could happen that the variable will be outdated inside the useEffect's body or undefined. Most likely it won't happen in this case because we initialize the store variables before the hook, but it's not a good practice in general. Moreover, there might be a race condition. If `specUrl` changes to frequently then request B may finish earlier then request A, in this case the outdated data will be used instead of the fresh. It won't happen in our case, since specUrl is either `null` and we don't fetched, or defined. ########## src/components/SwaggerUI/index.js: ########## @@ -0,0 +1,83 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React, { useState, useEffect } from 'react'; +import SwaggerUI from 'swagger-ui-react'; +import 'swagger-ui-react/swagger-ui.css'; +import useBaseUrl from '@docusaurus/useBaseUrl'; +import styles from './styles.module.css'; + +export default function SwaggerUIComponent({ spec, defaultServer }) { + const specUrl = useBaseUrl(spec); + const [serverUrl, setServerUrl] = useState(defaultServer || 'http://localhost:9888'); + const [specData, setSpecData] = useState(null); Review Comment: These variables aren't used anywhere. Are they necessary? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
