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

Add ESM bundle for Kedro-Viz #2268

Merged
merged 3 commits into from
Feb 11, 2025
Merged

Add ESM bundle for Kedro-Viz #2268

merged 3 commits into from
Feb 11, 2025

Conversation

ravi-kumar-pilla
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla commented Feb 6, 2025

Description

Add an ESM Kedro-Viz bundle to be used directly in a html document.

Development notes

  • Created a folder esm in the current kedro-viz GH repository
  • Upload the production bundles kedro-viz.production.mjs to the esm folder
  • Automate the process of bundling and updating the bundles via make version, i.e., add a step to update this folder when we do a new release

QA notes

  • All tests should pass
  • Test locally to check if the esm bundle is produced by running either make version VERSION=10.3.0 or npm run build:esm. Discard all the changes after testing

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: ravi_kumar_pilla <[email protected]>
@ravi-kumar-pilla ravi-kumar-pilla changed the title Add ESM bundle to Kedro-Viz Add ESM bundle for Kedro-Viz Feb 6, 2025
@ravi-kumar-pilla ravi-kumar-pilla marked this pull request as ready for review February 6, 2025 19:29
@rashidakanchwala
Copy link
Contributor

rashidakanchwala commented Feb 10, 2025

this is awesome <3 , few questions around

  • u mentioned ESM wasn't working before, what change made it work ?
  • the current ESM bundle size is 1.5MB, which is fine, but u mentioned ESM is in KBs -- so curious why?
  • u mentioned lodash, and react-dom as heavy dependencies as well, does it make sense to put them in external too?
  • u mentioned adding stack traces and we decided not to, as the bundle size for UMD grew very large. is this the case with ESM too?

I'm asking out of curiosity. We can continue improving our bundle sizes iteratively as we move forward :D

@astrojuanlu
Copy link
Member

Thanks @ravi-kumar-pilla ! I tested this HTML with the bundle and it worked 👏🏼

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Kedro-Viz</title>
  </head>
  <body>
    <div id="kedro-viz-1234" style="height: 600px"></div>
    <script src="https://cdn.jsdelivr.net/npm/[email protected]/umd/react.production.min.js"></script>
    <!-- script src="https://cdn.jsdelivr.net/npm/[email protected]/umd/react-dom.production.min.js"></script -->
    <script type="module">
      import { KedroViz, createRoot } from './kedro-viz.production.mjs';

      const viz_container = document.getElementById('kedro-viz-1234');
      const options = {
        display: {
          expandPipelinesBtn: false,
          exportBtn: false,
          globalNavigation: false,
          labelBtn: false,
          layerBtn: false,
          metadataPanel: true,
          miniMap: false,
          sidebar: false,
          zoomToolbar: false,
        },
        expandAllPipelines: false,
        behaviour: {
          reFocus: false,
        },
        theme: 'dark',
      };

      fetch('spaceflights.mock.json')
        .then((response) => response.json())
        .then((json) => {
          if (createRoot && viz_container) {
            const viz_root = createRoot(viz_container);
            viz_root.render(
              React.createElement(KedroViz, {
                data: json,
                options: options,
              })
            );
          }
        })
        .catch((error) => console.error('Error loading data:', error));
    </script>
  </body>
</html>
  • I did npm run build:esm and I see a diff in the esm folder. Is the expectation that the result should be identical to what's committed?
    • Related, should it be committed at all?
  • I tried to not use createRoot from the bundle but rather from react-dom, to no avail. In line with what @rashidakanchwala says, is there a chance we can make react-dom external too?

@ravi-kumar-pilla
Copy link
Contributor Author

  • u mentioned ESM wasn't working before, what change made it work ?

Frankly I was missing 2 lines in the config which says library:type:module and experiments:outputModule:true

  • the current ESM bundle size is 1.5MB, which is fine, but u mentioned ESM is in KBs -- so curious why?

Yes, when I was going thru this over the internet everyone said esm bundles are in kbs but it is the same for umd with additional optimizations (which I did earlier). ESM provides optimizations by default. I do not see any difference between UMD + additional optimization and ESM.

  • u mentioned lodash, and react-dom as heavy dependencies as well, does it make sense to put them in external too?

We can put them in external to make the bundle size small. I did that and it went to 1.3MB. I discussed this with @jitu5 and after testing with other versions of React, our bundle did not work. So we decided to bundle everything together. The advantage of doing this is, if any user wants to run viz directly in HTML as @astrojuanlu did, they just need to refer to 1 script. But if we make them external, they need to install external dependencies separately which will increase the complexity of using the bundle and also it might not work (for example React19 does not work). The disadvantage is it increases the bundle size.

For our current use case, I think it is better we go with a unified bundle and see if this creates any issues of users complaining of slowness.

  • u mentioned adding stack traces and we decided not to, as the bundle size for UMD grew very large. is this the case with ESM too?

Yes, I think UMD and ESM take the same space. Only difference I saw was ESM provides optimization like tree shaking by default with modern module syntax (import/export vs require)

I'm asking out of curiosity. We can continue improving our bundle sizes iteratively as we move forward :D

For sure. This would be a starting point and we can improve on the size by examining our node_modules.

Thank you

@ravi-kumar-pilla
Copy link
Contributor Author

Hi @astrojuanlu , Glad it worked :D

  • I did npm run build:esm and I see a diff in the esm folder. Is the expectation that the result should be identical to what's committed?

Yes, it is expected. npm run build:esm should be run only with a new release which will update our bundle. So I added it to make version command.

  • Related, should it be committed at all?

We need our bundle to be hosted somewhere which can be served via CDN. I could find 3 places where we can have this -

  1. npm package - Having a bundle here would increase our npm package size which is already quite big
  2. Our GitHub repo - This is a low effort task and we can easily automate with the release workflow. Having it in repo does not effect our users (npm install or pip install wont have this bundle)
  3. S3 bucket/Any cloud provider - A bit of effort and need to make changes to our CI workflow + maintenance of AWS/any cloud account.
  • I tried to not use createRoot from the bundle but rather from react-dom, to no avail. In line with what @rashidakanchwala says, is there a chance we can make react-dom external too?

As mentioned in the reply above - #2268 (comment)

Thank you

@rashidakanchwala rashidakanchwala self-requested a review February 10, 2025 15:27
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

thanks Ravi!! :) let's merge! ⭐

Signed-off-by: ravi_kumar_pilla <[email protected]>
@astrojuanlu
Copy link
Member

Thanks for the clarifications!

My only last comment is that maybe we can indeed publish the bundle to GitHub Releases without having to commit the bundle to the main branch, right?

Other than that, I'll let one more engineer review this 👍🏼

@ravi-kumar-pilla
Copy link
Contributor Author

ravi-kumar-pilla commented Feb 10, 2025

My only last comment is that maybe we can indeed publish the bundle to GitHub Releases without having to commit the bundle to the main branch, right?

I tried that @astrojuanlu uploading the bundle as an artifact in the github release. But I was unable to refer it via CDN like - https://cdn.jsdelivr.net/gh/kedro-org/[email protected]/esm/kedro-viz.production.mjs. I do see some articles suggesting that. Let me try one more time. If anyone did this before, please let me know @rashidakanchwala @ankatiyar @SajidAlamQB . Thanks

@ravi-kumar-pilla
Copy link
Contributor Author

My only last comment is that maybe we can indeed publish the bundle to GitHub Releases without having to commit the bundle to the main branch, right?

I tried publishing individual files in the release assets but these are not served on jsdelivr or other CDN. Only files that are committed can be served. I used GitHub native url but it is blocked due to CORS and not suitable for modules. I think we can go with the current approach and modify in future if there is a better alternative @astrojuanlu

Thank you

Signed-off-by: ravi_kumar_pilla <[email protected]>
Copy link
Contributor

@jitu5 jitu5 left a comment

Choose a reason for hiding this comment

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

@ravi-kumar-pilla ravi-kumar-pilla merged commit 4601642 into main Feb 11, 2025
12 checks passed
@ravi-kumar-pilla ravi-kumar-pilla deleted the feat/esm-viz-bundle branch February 11, 2025 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants