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

PostMessage without serialization #903

Merged

Conversation

Elephmoon
Copy link
Contributor

Resolve #897
I searched through the files in the frontend, in all places where I found postMessage removed unnecessary serialization.
Also, I think if we haven't stringify, then we don't need to check for string type here.

const data = typeof event.data === 'string' ? JSON.parse(event.data) : event.data;

@Elephmoon Elephmoon requested a review from umputun as a code owner March 4, 2021 05:07
@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #903 (54679a2) into master (6d4be02) will increase coverage by 0.00%.
The diff coverage is 3.84%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #903   +/-   ##
=======================================
  Coverage   43.98%   43.99%           
=======================================
  Files         127      127           
  Lines        2903     2905    +2     
  Branches      662      656    -6     
=======================================
+ Hits         1277     1278    +1     
- Misses       1613     1614    +1     
  Partials       13       13           
Impacted Files Coverage Δ
frontend/app/common/types.ts 100.00% <ø> (ø)
frontend/app/common/user-info-settings.ts 0.00% <ø> (ø)
frontend/app/components/auth-panel/auth-panel.tsx 78.16% <0.00%> (-0.91%) ⬇️
frontend/app/components/root/root.tsx 0.00% <0.00%> (ø)
frontend/app/embed.ts 0.00% <0.00%> (ø)
frontend/app/utils/postMessage.ts 0.00% <0.00%> (ø)
frontend/app/components/dropdown/dropdown.tsx 25.25% <20.00%> (+0.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1ef3ff...54679a2. Read the comment docs.

@akellbl4 akellbl4 requested review from akellbl4 and Mavrin March 4, 2021 08:55
@akellbl4
Copy link
Collaborator

akellbl4 commented Mar 4, 2021

Also, I think if we haven't stringify, then we don't need to check for string type here.

Yes, it's a good point, but for example, in dev, webpack could send string inside post message data.
In my opinion, we should drop JSON.parse for data from postMessage and process message if it's an object.
Good to add these changes in current PR.

@Elephmoon
Copy link
Contributor Author

Elephmoon commented Mar 7, 2021

we should drop JSON.parse for data from postMessage

I'm not very good in typescript and I can't catch how to make it.
@akellbl4 can you help me?

const data = typeof event.data === 'string' ? JSON.parse(event.data) : event.data;

In this line JSON.parse implicitly changing type to any and in next line we can get data.theme.
If I remove JSON.parse data type will not change and we will not get data.theme.
I need to define type with Theme type or what is the best way in this situation?

@akellbl4
Copy link
Collaborator

akellbl4 commented Mar 9, 2021

@Elephmoon we already removed JSON.stringify on sending. It means we will receive JS object.
All we need to check if data is an object and work with it. Just like that:

onMessage(event: MessageEvent<{ theme?: string }>) {
    const isObject = typeof event.data === 'object' && event.data !== null && !Array.isArray(event.data)
    if (!isObject) {
        return
   }
   // work with data
}

We set MessageEvent type to the event and wait for { theme?: string }. We don't sure what we will get because we add such check on evt.data
I don't like the way how it's should be checked but it's js. @Mavrin maybe you know cool methods to do that?

@akellbl4
Copy link
Collaborator

@Elephmoon we have the same data processing from post message in embed.ts and other places.

const data = typeof event.data === 'string' ? JSON.parse(event.data) : event.data;

const data = typeof event.data === 'string' ? JSON.parse(event.data) : event.data;

const data = typeof e.data === 'string' ? JSON.parse(e.data) : e.data;

@Elephmoon Elephmoon requested a review from akellbl4 March 10, 2021 18:55
akellbl4
akellbl4 previously approved these changes May 2, 2021
Copy link
Collaborator

@akellbl4 akellbl4 left a comment

Choose a reason for hiding this comment

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

Sorry for the long response.
Thank you for your contribution!

@akellbl4 akellbl4 force-pushed the post-message-without-serialization branch from 1728b35 to 044294a Compare May 3, 2021 19:42
@akellbl4
Copy link
Collaborator

akellbl4 commented May 3, 2021

@Mavrin could you please review the PR? I've added some additional code, could be cool to have second opinion on it :)

Mavrin
Mavrin previously approved these changes May 4, 2021
Copy link
Collaborator

@Mavrin Mavrin left a comment

Choose a reason for hiding this comment

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

lgtm

@umputun
Copy link
Owner

umputun commented May 4, 2021

@Elephmoon can you rebase all of this into a single commit so I could merge it nicely?

@akellbl4
Copy link
Collaborator

@Elephmoon hi, do you need help with it?

@Elephmoon
Copy link
Contributor Author

Elephmoon commented May 16, 2021

@Elephmoon hi, do you need help with it?

Sorry. I’m moving to other city right now, I believe that I have time today evening and I will solve it

@Elephmoon Elephmoon force-pushed the post-message-without-serialization branch 2 times, most recently from 33ddb9f to faaccba Compare May 16, 2021 11:19
@Elephmoon
Copy link
Contributor Author

Ugh. I made incorrect rebase and lost all changes (;

@akellbl4
Copy link
Collaborator

@Elephmoon I'll try to revert it ;)

@akellbl4 akellbl4 force-pushed the post-message-without-serialization branch 2 times, most recently from 044294a to 0a98982 Compare May 16, 2021 17:45
@akellbl4
Copy link
Collaborator

@Elephmoon I've fixed it, Is it okay for you to merge it with squash?

@Elephmoon Elephmoon force-pushed the post-message-without-serialization branch from 0a98982 to 54679a2 Compare May 16, 2021 18:29
@Elephmoon
Copy link
Contributor Author

Yep. I squashed it

@akellbl4
Copy link
Collaborator

@umputun good to merge ;)

@umputun umputun merged commit 992b843 into umputun:master May 16, 2021
@paskal paskal added this to the v1.9 milestone Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Post data via post message without serialization
6 participants