-
Notifications
You must be signed in to change notification settings - Fork 8
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! 👋🏼
|
|
@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