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.

Sign up for GitHub

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

Jump to bottom

fix: vue 3 log warning causes error on iOS #6993

Merged
merged 10 commits into from
Mar 22, 2024
Merged

fix: vue 3 log warning causes error on iOS #6993

merged 10 commits into from
Mar 22, 2024

Conversation

ciekawy
Copy link
Contributor

@ciekawy ciekawy commented Oct 14, 2023 •
edited
Loading

fixes #6615
not sure what exactly should be returned when String throws error

@markemer markemer changed the title fix for #6615 fix: vue 3 log warning causes error on iOS (#6615) Jan 18, 2024
@markemer markemer changed the title fix: vue 3 log warning causes error on iOS (#6615) fix: vue 3 log warning causes error on iOS Jan 18, 2024
@markemer markemer self-assigned this Jan 18, 2024
markemer
markemer reviewed Jan 18, 2024
View reviewed changes
core/native-bridge.ts Outdated
return String(msg);
}
catch (e) {
return '??';
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably return a blank string, if that will also avoid the error. Or perhaps an error message to the console and a blank string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, seems I left the ?? as was not sure what should be returned. I'm definitely ok with empty string unless there is any better suggestion.

markemer
markemer requested changes Jan 18, 2024
View reviewed changes
Copy link
Contributor

@markemer markemer left a comment

Choose a reason for hiding this comment

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

Suggestion

@markemer
Copy link
Contributor

markemer commented Feb 5, 2024

Can you run npm run fmt as well - appears to be failing lint checks.

@kupcza kupcza mentioned this pull request Feb 27, 2024
Closed
@ciekawy
Copy link
Contributor Author

ciekawy commented Mar 7, 2024 •
edited
Loading

I managed to quickly sync, do the fmt and push
just letting know that unfortunately I'm very time constrained now and for next several weeks. I'd be happy to update the PR, there is a chance I'll be soon touching an app with this and will try to be back (to not left the app blocked I just used patch-package at that time)

@ciekawy ciekawy requested a review from markemer March 7, 2024 15:32
@ciekawy
Copy link
Contributor Author

ciekawy commented Mar 20, 2024

@markemer I addressed your suggestions

@jcesarmobile jcesarmobile merged commit 87271e2 into ionic-team:main Mar 22, 2024
6 checks passed
jcesarmobile added a commit that referenced this pull request Mar 22, 2024
@ciekawy @markemer @jcesarmobile
fix: vue 3 log warning causes error on iOS (#6993)
67a3de7 
Co-authored-by: Mark Anderson <mark@ionic.io>
Co-authored-by: jcesarmobile <jcesarmobile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markemer markemer markemer approved these changes

@jcesarmobile jcesarmobile jcesarmobile approved these changes

@theproducer theproducer Awaiting requested review from theproducer

@nflsilva nflsilva Awaiting requested review from nflsilva

@giralte-ionic giralte-ionic Awaiting requested review from giralte-ionic

Assignees

@markemer markemer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: log vue 3 warn cause error on iOS
3 participants
@ciekawy @markemer @jcesarmobile