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

Encode and decode copy-to-clipboard functionality using base64 #372

Merged
merged 1 commit into from
May 15, 2023

Conversation

xenova
Copy link
Contributor

@xenova xenova commented May 12, 2023

Fixes issue where the code attribute for copy-to-clipboard functionality is interpreted as valid Svelte code, which leads to SSR errors.

@xenova
Copy link
Contributor Author

xenova commented May 12, 2023

The failed tests seem to be related to a recent update of timm, and shouldn't have anything to do with this PR.

@coyotte508
Copy link
Member

Can you give an example of markdown that fails with current code?

@xenova
Copy link
Contributor Author

xenova commented May 13, 2023

Can you give an example of markdown that fails with current code?

Sure thing - sorry, I had mentioned to @mishig25 privately.

# This is test\n\n
```jsx
let worker = new Worker(new URL('./worker.js', import.meta.url));

@coyotte508
Copy link
Member

coyotte508 commented May 13, 2023

if you add a newline between #this is test and the code sample, does the problem still happen?

Edit ok, tested and it doesn't work:
image

Trying with your branch

Edit 2: Same error. PR: https://github.com/huggingface/huggingface.js/pull/192/files

@xenova
Copy link
Contributor Author

xenova commented May 13, 2023

That's odd; it works locally.

Here's the original version rendered correctly:

image

I did notice that, when testing locally, it sometimes uses a cached version of the doc builder.

Edit: I see it's actually producing a different error, and it's trying to evaluate the worker instantiation.

@coyotte508
Copy link
Member

Ok after a few tries your fix worked! :) (github action cache...)

Approving, leaving it up to @mishig25 to take at second look & merge next week

@coyotte508 coyotte508 requested a review from mishig25 May 13, 2023 19:52
mishig25 added a commit to huggingface/transformers that referenced this pull request May 15, 2023
Copy link
Contributor

@mishig25 mishig25 left a comment

Choose a reason for hiding this comment

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

lgtm!
I've tested the build on transformers docs & everything is working as expected huggingface/transformers#23361

Therefore, merging the PR 🚀

(test is failing for unrelated reason to this PR)

@mishig25 mishig25 merged commit c59346e into huggingface:main May 15, 2023
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.

3 participants