-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: update dependencies #212
base: master
Are you sure you want to change the base?
Conversation
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.
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.
@@ -51,9 +51,6 @@ export async function transpileFiles(directory: string, outputDir: string, optio | |||
sourcemap: true, | |||
dir: outputDir, | |||
exports: "auto", | |||
paths: { |
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.
In react@18
the production build is conditionally exported based on the value of NODE_ENV
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.
Also, the react/cjs/react-jsx-runtime.production.min
file can no longer be resolved because the file is not part of the react
package.json#exports
// check substring of the desired error | ||
expect((error as Error).message).toContain('Invalid hook call.'); | ||
// A console.error() will be printed due to illegal use of hooks but only in development mode | ||
expect(() => render(<Component />)).toThrow("Cannot read properties of null (reading 'useState')"); |
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.
In react@18
this no longer throws with "Invalid hook call."
.
Instead it prints the error via console.error()
but only in development mode
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.
@leon19 thanks for taking the time to provide this!
I have a question I hope you can help clarify ✌️ Since we are jumping a few major versions in some of the dependencies, notably in the react
dependency. Will existing templates (written with React 17 in mind) be backward compatible with this change?
@jonaslagoni I tested it at work with Sadly, I have tried to set it up locally with WIll try to find out if the upgrade is possible when I have the time 👌 |
I would probably try with just Generator and HTML-template (skipping CLI for simplicity) and using NPM link 🤔 Definitely need that test to go through 😄 |
@jonaslagoni I was able to test it as you mentioned. It failed 😅 This change is needed in the template to get it to work export function renderSpec(asyncapi, params) {
loadLanguagesConfig();
const config = prepareConfiguration(params);
const stringified = stringifySpec(asyncapi);
const component = <AsyncApiComponent schema={stringified} config={config}/>;
- return ReactDOMServer.renderToString(component);
+ return ReactDOMServer.renderToString(() => component);
} The good news is that by updating only that line, the generator still works with But yes, this PR cannot be merged before applying the change in the HTML template |
2ebd138
to
b7d867f
Compare
Hello, @leon19! 👋🏼
|
Quality Gate passedIssues Measures |
@leon19 so looks like that even if we fix HTML template, the upgrade is a breaking change for generator that depends on react sdk |
Description
Related issue(s)
Fixes #208