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(neffos.js): only conditionally import node websocket dependency #33

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alpjor
Copy link
Contributor

@alpjor alpjor commented Jan 8, 2024

No description provided.

@alpjor alpjor force-pushed the alpjor/conditional-import branch from 80efd36 to 7dce121 Compare January 9, 2024 23:00
@kataras
Copy link
Owner

kataras commented Jan 10, 2024

Is it tested @alpjor?

@alpjor
Copy link
Contributor Author

alpjor commented Jan 10, 2024

@kataras I've tested this version running both in browser contexts and in a node env. I'm a little concerned about changing the target: and module: because I don't know if that'll change the support for older browsers where I haven't done any testing, but those changes where necessary to get access to the await import( syntax to conditionally load the dependency. It's a bit weird though, because before the PR you accepted of mine about the nonce, the conditional load code was there and was working, but after my PR it was gone. I think there might've been some changes in the dist folder that weren't reflected in the src folder. I looked at the compiled code in the dist folder from the commit before my change and it doesn't seem to 100% match the src code at the time (es5 and min versions were different from plain js too). Maybe a bad push? lmk what you think and thanks for looking at it. Either way, this code does work for me in my fork in all my testing in modern browser environments, I hope it helps here too

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.

2 participants