Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leon19
Copy link

@leon19 leon19 commented Mar 13, 2024

Description

  • Update dependencies

Related issue(s)

Fixes #208

Copy link

@github-actions github-actions bot left a 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: {
Copy link
Author

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

Copy link
Author

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')");
Copy link
Author

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

@leon19 leon19 changed the title Update dependencies feat: update dependencies Mar 13, 2024
Copy link
Member

@jonaslagoni jonaslagoni left a 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?

@leon19
Copy link
Author

leon19 commented Mar 20, 2024

@jonaslagoni I tested it at work with react@18 and it worked, for some reason it got installed instead of react@17 and it was breaking our build. I used patch-package to add the export to the [email protected] file and it worked just fine.

Sadly, I have tried to set it up locally with @asyncapi/cli and @asyncapi/html-template and I'm not able to get it to work 😞

WIll try to find out if the upgrade is possible when I have the time 👌

@jonaslagoni
Copy link
Member

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 😄

@leon19
Copy link
Author

leon19 commented Mar 22, 2024

@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 react@17

But yes, this PR cannot be merged before applying the change in the HTML template

@asyncapi-bot
Copy link
Contributor

Hello, @leon19! 👋🏼

    I'm 🧞🧞🧞 Genie 🧞🧞🧞 from the magic lamp. Looks like somebody needs a hand!

    At the moment the following comments are supported in pull requests:

    - `/ready-to-merge` or `/rtm` - This comment will trigger automerge of PR in case all required checks are green, approvals in place and do-not-merge label is not added
    - `/do-not-merge` or `/dnm` - This comment will block automerging even if all conditions are met and ready-to-merge label is added
    - `/autoupdate` or `/au` - This comment will add `autoupdate` label to the PR and keeps your PR up-to-date to the target branch's future changes. Unless there is a merge conflict or it is a draft PR.

Copy link

sonarcloud bot commented Mar 22, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@derberg
Copy link
Member

derberg commented Aug 11, 2024

@leon19 so looks like that even if we fix HTML template, the upgrade is a breaking change for generator that depends on react sdk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update dependencies.
4 participants