feat: switch-to-jsx-renderer #316
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
|
@magicmatatjahu I have made index.js as requested kindly review the PR.😁 |
There was a problem hiding this comment.
@ritik307 Hello! I made review. Good start, but we need some things to improve :)
Please, don't be horrified by these comments :) I gave hints what you should improve. Remember an important thing: the new template should work similarly (create the same file) as the previous template (written in Nunjucks). You can generate an old template and modify the new one to get the same result to check that everything is ok. It actually has to work the same, otherwise we will not be able to merge it.
|
Hi @magicmatatjahu I have implemented the changes you have mentioned but I am confused about how to test them :( |
There was a problem hiding this comment.
@ritik307 If you wanna test it, you should read that section https://github.com/asyncapi/template-for-generator-templates#optional-tests-for-each-feature-of-the-template and here you have examples how to do it https://github.com/asyncapi/markdown-template/blob/master/test/components/Bindings.test.js Write if you will have problems :)
|
@magicmatatjahu I ma made the requested changes kindly look at them :) |
There was a problem hiding this comment.
@ritik307 Better, but we still need to improve few things :) Awesome progress 🚀
|
@magicmatatjahu Made the requested changes kindly look into them :) |
components/common.js
Outdated
| const links = cssLinks | ||
| .map((link) => `<link rel="stylesheet" href="${link}">\n`); | ||
|
|
||
| const content = ` | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| ${base && `<base href=${base}>`} | ||
| <title>${title}</title> | ||
| ${withIndendation(links, 2, IndentationTypes.SPACES)} | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| ${(styleContent.length > 0) ? `<style>\n${styleContent[0]}\n${styleContent[1]}</style>` : ""} | ||
| ${(cssLinks.length > 0) ? `\n<link href=${cssLinks[0]}>\n<link href=${cssLinks[1]}>\n` : ""} |
There was a problem hiding this comment.
You have performed links and saved in the links array. Why you do it again in the 28 line? :)
There was a problem hiding this comment.
basically, we have cssLinks array and styleContent array so if I am thinking correctly we will only add styleContent when the array is not empty and the same goes for cssLinks so according to me we have to check them separately. :)
There was a problem hiding this comment.
Yeah, you understand it correctly, but please remove:
const links = cssLinks
.map((link) => `<link rel="stylesheet" href="${link}">\n`);
because currently it's unnecessary :)
|
@ritik307 Could you fix https://sonarcloud.io/project/issues?id=asyncapi_html-template&pullRequest=316&resolved=false&types=CODE_SMELL problems? :) You should move that interpolation strings to outside as save in variables and then reuse them in final string. |
|
@magicmatatjahu Made the changes. Kindly review :) |
There was a problem hiding this comment.
Overall good :) We need to fix some things because we have broken template. Also I wrote about missmatch between ESM and CJS modules in filters/all file, so please change content of that file to:
import fs from 'fs';
import path from 'path';
import React from 'react';
import ReactDOMServer from 'react-dom/server';
import AsyncApiComponent, { hljs } from '@asyncapi/react-component';
import { AsyncAPIDocument } from '@asyncapi/parser';
/**
* Prepares configuration for component.
*/
function prepareConfiguration(params = {}) {
const config = { show: { sidebar: true }, sidebar: { showOperations: 'byDefault' } };
if (params.sidebarOrganization === 'byTags') {
config.sidebar.showOperations = 'bySpecTags';
}
if (params.sidebarOrganization === 'byTagsNoRoot') {
config.sidebar.showOperations = 'byOperationsTags';
}
return config;
}
let initLanguages = false;
/**
* Load all language configurations from highlight.js
*/
function loadLanguagesConfig() {
if (initLanguages === true) {
return;
}
/**
* Retrieve the location of highlight.js.
* It's needed because someone can have installed `highlight.js` as global dependency
* or depper than local `node_modules` of this template.
*/
const hljsPackageDir = path.dirname(require.resolve("highlight.js/package.json"))
const hljsLanguagesPath = path.resolve(hljsPackageDir, 'lib/languages');
const languages = fs.readdirSync(hljsLanguagesPath);
for (let langPath of languages) {
const lang = require(path.resolve(hljsLanguagesPath, langPath.replace('.js', '')));
hljs.registerLanguage(lang.name, lang);
}
initLanguages = true;
}
/**
* More safe function to include content of given file than default Nunjuck's `include`.
* Attaches raw file's content instead of executing it - problem with some attached files in template.
*/
export function includeFile(pathFile) {
const pathToFile = path.resolve(__dirname, '../', pathFile);
return fs.readFileSync(pathToFile);
}
/**
* Stringifies the specification with escaping circular refs
* and annotates that specification is parsed.
*/
export function stringifySpec(asyncapi) {
return AsyncAPIDocument.stringify(asyncapi);
}
/**
* Stringifies prepared configuration for component.
*/
export function stringifyConfiguration(params) {
return JSON.stringify(prepareConfiguration(params));
}
/**
* Renders AsyncApi component by given AsyncAPI spec and with corresponding template configuration.
*/
export function renderSpec(asyncapi, params) {
loadLanguagesConfig();
const component = React.createElement(AsyncApiComponent, { schema: asyncapi, config: prepareConfiguration(params) });
return ReactDOMServer.renderToString(component);
}
|
@magicmatatjahu Made changes. Kindly review them :) |
There was a problem hiding this comment.
Last thing to fix: https://sonarcloud.io/project/issues?id=asyncapi_html-template&open=AX7pQCNwRTpeUDRjCec4&pullRequest=316&resolved=false&types=CODE_SMELL We need to remove that nested interpolation string. You can make something like:
const styleContentData = styleContent.length ? `<style type="text/css">${styleContent.join('\n')}</style>` : '';and then reuse it:
${styleContentData ? styleContentData : ''}
|
@ritik307 Before merging that PR, we need first to resolve that problem asyncapi/generator#521 I will try to focus on it in this week, so that PR atm will be annotated as |
|
Kudos, SonarCloud Quality Gate passed!
|
Got it 👍 |
|
@magicmatatjahu Is the linked issue resolved? 😊 |
|
@ritik307 If issue is still opened, then no :) It will be closed, when we will merge PR with fix. |
|
This pull request has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation. There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model. Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
|
This pull request has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation. There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model. Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
|
Still needs asyncapi/generator#521 |
|
This pull request has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation. There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model. Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
Description
template/index.htmltotemplate/index.jsRelated issue(s)
Resolves #185