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

fix(superset-embedded-sdk): Buffer is not defined #21641

Merged
merged 2 commits into from
Oct 17, 2022

Conversation

doornot
Copy link
Contributor

@doornot doornot commented Sep 29, 2022

Buffer is a Nodejs api, doesn't support in the browser environment. Replace Buffer with jwt-decode to parse Jwt.

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@doornot
Copy link
Contributor Author

doornot commented Oct 11, 2022

hello @villebro, please help review.

BTW, if this pr be approved and merged, will CI publish @superset-ui/embedded-sdk automatically?

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM, but not sure if we should bump the version in this PR. Also, I'm wondering if we should include this library in monorepo - WDYT @zhaoyongjie ?

@@ -1,6 +1,6 @@
{
"name": "@superset-ui/embedded-sdk",
"version": "0.1.0-alpha.7",
"version": "0.1.0-alpha.8",
Copy link
Member

Choose a reason for hiding this comment

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

I think it's best not to bump this in this PR, but rather when the package is published. WDYT @zhaoyongjie ?

Copy link
Member

Choose a reason for hiding this comment

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

@doornot Could you revert this change and package-lock.json so that I can merge it even if CI reports that issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks @zhaoyongjie

@zhaoyongjie zhaoyongjie self-requested a review October 12, 2022 07:23
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

Codes LGTM, the browser supported ArrayBuffer rather than Buffer.

Right now, the frontend code has 3 parts, superset-frontend, superset-embedded-sdk, and superset-websocket. In order to auto-publish these, Moving to the monorepo might be a good idea.

@doornot
Copy link
Contributor Author

doornot commented Oct 13, 2022

I would like to know how can i fix the failed merge check("Welcome New Contributor / welcome (pull_request_target)")? @zhaoyongjie @villebro

@mayurnewase mayurnewase linked an issue Oct 14, 2022 that may be closed by this pull request
3 tasks
@villebro villebro merged commit 7ec136f into apache:master Oct 17, 2022
@andrey-hohlov
Copy link

andrey-hohlov commented Nov 28, 2022

Any chance to get new release with these changes?

@villebro
Copy link
Member

Any chance to get new release with these changes?

@andrey-hohlov that's a good idea. In the future new versions should be released when new versions come out, but for now I think we should be able to do a manual release to make the most recent changes available. @zhaoyongjie have you done releases to the embedded SDK lately? If not I can take a stab at it..

@zhaoyongjie
Copy link
Member

@villebro I haven't touch embedded SDK recently.

@aehanno
Copy link
Contributor

aehanno commented Dec 21, 2022

Hi ! Do you know when there will be a release with this bug ?

@erik-squared
Copy link

any chance there will be a new release in the near future? if not, is there a workaround for a browser environment?

@andrey-hohlov
Copy link

any chance there will be a new release in the near future? if not, is there a workaround for a browser environment?

@eduus710 we use this for now:

https://www.npmjs.com/package/buffer

import { Buffer } from 'buffer';

window.Buffer = Buffer;

@villebro
Copy link
Member

FYI I will try to push a new version of the SDK out today, I will keep you posted.

@villebro
Copy link
Member

villebro commented Jan 11, 2023

Can you check if this is working as expected? https://www.npmjs.com/package/@superset-ui/embedded-sdk/v/0.1.0-alpha.8 (switchboard also published: https://www.npmjs.com/package/@superset-ui/switchboard/v/0.18.26-1)

@erik-squared
Copy link

thank you!

(fyi - i grabbed the bundle from npm; however, i notice that unpkg.com is still serving alpha 7)

@andrey-hohlov
Copy link

@villebro Thank you, works for us

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embedded-SDK getGuestTokenRefreshTiming missing buffer.from
7 participants