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

chore: add esm, cjs and umd bundles #297

Merged
merged 6 commits into from
Apr 16, 2021

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented Apr 12, 2021

Description

Changes proposed in this pull request:

  • Add esm bundle under module field,
  • Add cjs bundle under main field,
  • Add umd bundle under {package}/bundles/umd directory.
  • Update package.json with helper scripts.
  • switch from dompurify to isomorphic-dompurify to support rendering in node and in browser - original dompurify can only be used in browser.
  • extract main component to two separates: AsyncAPI with parser and Standalone without parser -- for these two bundler creates two umd packages.
  1. ems is prepared when you use something like create-react-app when someone have another bundler (webpack) to bundle our component with another application. Only for better DX.
  2. cjs is prepared only for application when you use CommonJS (require, module.exports). There is also not needed to bundle our package by webpack, because on the Node app should deal with the dependencies of our component. cjs is only needed for rendering on the server side (hydration), when someone use cjs, otherwise will be used esm.
  3. umd can be used on Node side and client side (browser), but unfornatelly we are using parser in our component which needs a Node polyfills to run on browser, so umd is only needed when someone wants to run a component as a script in html - something like webcomponent but run from react. For umd I created two packages, with and without parser, but you know why we need it :)

Related issue(s)
Part of asyncapi/shape-up-process#105

@magicmatatjahu magicmatatjahu added enhancement New feature or request area/library Related to all activities around Library package labels Apr 12, 2021
@magicmatatjahu magicmatatjahu requested a review from derberg April 16, 2021 10:27
},

plugins: [
// new BundleAnalyzerPlugin(),
Copy link
Member Author

@magicmatatjahu magicmatatjahu Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BundleAnalyzerPlugin is used to check size of bundled package on prepared for that html-site. I don't wanna remove it from code, but only comment when someone will want to check the size before commit (with new changes in component).

@@ -0,0 +1,146 @@
import React, { Component } from 'react';
Copy link
Member Author

@magicmatatjahu magicmatatjahu Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the old AsyncAPI component but without parser. "New" AsyncAPI component uses this component directly in render function to avoid repetitive code.

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks fine, just one comment so I can understand something

@@ -153,7 +161,7 @@ export const Schema: React.FunctionComponent<Props> = ({
key={idx}
className="border inline-block text-orange-600 rounded ml-1 py-0 px-2"
>
<span>{e}</span>
<span>{JSON.stringify(e)}</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain why now we need to stringify this and const? I thought it is string always

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON.stringify add to string "", similar to Nunjucks something | safe. I think that I should make some helper to "prettiefy" these string, not using JSON.stringify.

@derberg
Copy link
Member

derberg commented Apr 16, 2021

I have to shamefully confess I don't get why it is so important to support all these different bundlers. It only makes me feel there are moments where I feel tired with JS ecosystem complexity 😄

@derberg
Copy link
Member

derberg commented Apr 16, 2021

also strange that with so many modifications there was no need to update/add any tests 😝

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Apr 16, 2021

@derberg Agree with you with JS ecosystem complexity! 😅 I will try to explain why we need it (for community of course):

  • ems is prepared when you use something like create-react-app when someone have another bundler (webpack) to bundle our component with another application. Only for better DX.
  • cjs is prepared only for application when you use CommonJS (require, module.exports). There is also not needed to bundle our package by webpack, because on the Node app should deal with the dependencies of our component. cjs is only needed for rendering on the server side (hydration), when someone use cjs, otherwise will be used esm.
  • umd can be used on Node side and client side (browser), but unfornatelly we are using parser in our component which needs a Node polyfills to run on browser, so umd is only needed when someone wants to run a component as a script in html - something like webcomponent but run from react. For umd I created two packages, with and without parser, but you know why we need it :)

Feel free when you have another questions!

also strange that with so many modifications there was no need to update/add any tests 😝

PR adds only configurations for the webpack, in which way you want to test it? 🤣

@derberg
Copy link
Member

derberg commented Apr 16, 2021

PR adds only configurations for the webpack, in which way you want to test it? 🤣

just wanted to point out I'm missing components tests in overal 😛

I will try to explain why we need it

thanks for that, please update description with this comment so it is clear for anyone in the future and there is no need to scroll through comments

I also think we need some info about it in the readme, don't you think?

btw, you think it will solve issue with next.js? as a side effect :D

@magicmatatjahu
Copy link
Member Author

@derberg I'm kidding with tests, I know about it, at the moment I need only this PR, some changes with styling, updated Readme and I'm on the way to merge to master. I think that on Monday I will have the last PR to merge before merging to master and I will think about missed tests to helpers. Component testing 🤔 I don't know if it's needed, because we don't have any interactivity in them.

btw, you think it will solve issue with next.js? as a side effect :D

I tested umd module in studio and it works, so yeah, as a side effect I fixed these problems with next.js, but at the moment, I don't want to write about it in these issues.

@derberg
Copy link
Member

derberg commented Apr 16, 2021

you mean you want me to approve and update readme in final PR?

@magicmatatjahu
Copy link
Member Author

I will write helper to prettify string from const, enum and I will ping you to accept PR. I mean about Readme.md that it will be included in next PR (probably the last one).

@derberg
Copy link
Member

derberg commented Apr 16, 2021

ok, but please remember about the explanation for different modules, as I will forget to check that explicitly

also, hint for final PR -> would be awesome to have playground deployed from the branch, somewhere, for manual testing + webcomponent available for test too

@magicmatatjahu
Copy link
Member Author

ok, but please remember about the explanation for different modules, as I will forget to check that explicitly

👌🏼

also, hint for final PR -> would be awesome to have playground deployed from the branch, somewhere, for manual testing + webcomponent available for test too

I planned to add instruction how to install html-template with new component onboard something like ag {spec} ../{my branch} to test it.

@derberg
Copy link
Member

derberg commented Apr 16, 2021

I think deploying playground from your branch would make it super easy for others to take a look and try out their files, to give you a good feedback.

anyway, approving

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@magicmatatjahu
Copy link
Member Author

@derberg I added missed function. I see that I have possibility to merge, so I'm merging :) Thanks for review!

@magicmatatjahu magicmatatjahu merged commit 320c389 into asyncapi:unify-component Apr 16, 2021
@magicmatatjahu magicmatatjahu deleted the unify-bundles branch April 16, 2021 12:37
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.21.0-next.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/library Related to all activities around Library package enhancement New feature or request released on @next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants