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 keepNames to esbuild options #150

Merged
merged 5 commits into from
Aug 3, 2023
Merged

Add keepNames to esbuild options #150

merged 5 commits into from
Aug 3, 2023

Conversation

bananabrann
Copy link
Contributor

Hi @geoffrich, thanks for the work you do here. It's a little change, but I hope it's correct. I'm pretty new to SvelteKit, so admittedly I'm not quite sure how I could go about testing this adapter locally. Double whammy, I've also never contributed to anything open source before. If I need to do more, please just let me know and I'll learn and give a go this weekend 🏎️

My use case was mentioned in #148 --this is causing a bug that prevents me from logging entries on my grandma's TV app (that's secretly a SvelteKit website)

Closes #148
Mentioned in #51 #56

@tlaundal
Copy link
Contributor

tlaundal commented Aug 2, 2023

Change looks good! Perhaps also update the relevant README section?

@bananabrann bananabrann marked this pull request as ready for review August 2, 2023 12:31
Copy link
Owner

@geoffrich geoffrich left a comment

Choose a reason for hiding this comment

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

This looks good! Can you test this branch out with your project and confirm it fixes the issue? You can install this adapter from your GitHub branch with the following command:

npm i github:bananabrann/svelte-adapter-azure-swa#bananabrann/%23148-external-esbuild

README.md Outdated Show resolved Hide resolved
@bananabrann
Copy link
Contributor Author

Sure thing @geoffrich ! I'll try it out today
Thanks so much for including the command

README.md Outdated Show resolved Hide resolved
Co-authored-by: Tobias Laundal <tobias@laundal.no>
@bananabrann
Copy link
Contributor Author

Looks like these changes work for me 🥳 Here are some details that (hopefully) make it easy for you to confirm these changes are indeed what fixed it for me

Demo with forked adapter (working)

https://black-mushroom-06e3d7310-56.centralus.3.azurestaticapps.net/ask

Send a message a see that it works just fine
FYI, I receive your IP address and the text you typed, but I don't do anything with it

Demo of adapter ^0.16.0 (not working)

https://black-mushroom-06e3d7310-57.centralus.3.azurestaticapps.net/ask

Send a chat message, observe scary error

TypeError: Expected signal to be an instanceof AbortSignal

I have the bad site PR'd off the good site so you can see that the only change is the adapter

Copy link
Owner

@geoffrich geoffrich 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 this looks good, thanks for your PR and for testing it out! I'll release the next adapter version with the fix momentarily.

README.md Outdated Show resolved Hide resolved
@geoffrich geoffrich merged commit dbe44ba into geoffrich:main Aug 3, 2023
5 checks passed
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.

Add keepNames esbuild option
3 participants