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

Calypsoify-iframe: Janitorial types and fixes #39057

Closed
wants to merge 4 commits into from

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Jan 24, 2020

Some fixes and improvements to reduce type issues in calypsoify-iframe. It would be great to resurrect #32872

Inspired by #39048 (review)

  • Bare minimum JSDoc typings on MediaStore and MediaActions
  • Some lint tweaks and ignores
  • Use globalThis for browser globals.

Testing instructions

  • The iframe editor continues to work. There should be no behavioral changes. Be sure to check the media modal.

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Janitorial labels Jan 24, 2020
@sirreal sirreal requested review from a team as code owners January 24, 2020 13:43
@sirreal sirreal self-assigned this Jan 24, 2020
@matticbot
Copy link
Contributor

const {
port1: iframePortObject,
port2: transferredPortObject,
} = new globalThis.MessageChannel();
Copy link
Member Author

Choose a reason for hiding this comment

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

globalThis is a code change here.

Copy link
Contributor

Choose a reason for hiding this comment

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

How would you compare adding globalThis vs. something like /* global MessageChannel */ at the top of the file?

Im guessing there isn't any functional difference between the two and since we only have 1 instance of MessageChannel being called in the file it makes more sense to call it off the globalThis here explicitly?

@matticbot
Copy link
Contributor

matticbot commented Jan 24, 2020

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Async-loaded Components (~9063 bytes added 📈 [gzipped])

name                            parsed_size              gzip_size
async-load-layout-guided-tours     +33570 B  (+1166.4%)    +9063 B  (+879.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for taking care of these.

I am a little confused why these errors still seem to show up after building and running this PR:
Screen Shot 2020-01-27 at 4 45 10 PM

I was assuming the typedefs you added should clear that?

const {
port1: iframePortObject,
port2: transferredPortObject,
} = new globalThis.MessageChannel();
Copy link
Contributor

Choose a reason for hiding this comment

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

How would you compare adding globalThis vs. something like /* global MessageChannel */ at the top of the file?

Im guessing there isn't any functional difference between the two and since we only have 1 instance of MessageChannel being called in the file it makes more sense to call it off the globalThis here explicitly?

@sirreal sirreal force-pushed the update/calypsoify-iframe-types branch from bf158e9 to 3d9cbe5 Compare January 30, 2020 09:16
@sirreal
Copy link
Member Author

sirreal commented Jan 30, 2020

How would you compare adding globalThis vs. something like /* global MessageChannel */ at the top of the file?

This was dropped in rebase because it was handled in #38986. There are some details there: https://github.com/Automattic/wp-calypso/pull/38986/files#r369949284

In short, we' started to discourage usage of globals because there are too many and we've seen usage lead to bugs. They should be prefixed with window (browser-only) or globalThis (browser/web workers/node).

@sirreal
Copy link
Member Author

sirreal commented Jan 30, 2020

I am a little confused why these errors still seem to show up after building and running this PR […]
I was assuming the typedefs you added should clear that?

Indeed, I intended to fix the bare minimum while providing an example of how they can be added. They seem to be fixed in my editor, you may need to restart the language server, restart your editor, or force it to reparse the JSdoc in some way. In VS Code, there's a command TypeScript: Restart TS Server that will do the trick.

@sirreal
Copy link
Member Author

sirreal commented Jan 30, 2020

Rebased to fix conflicts.

@sirreal sirreal requested review from a team and Addison-Stavlo January 30, 2020 09:29
@sirreal
Copy link
Member Author

sirreal commented Jan 30, 2020

Landed in #39048

@sirreal sirreal closed this Jan 30, 2020
@sirreal sirreal deleted the update/calypsoify-iframe-types branch January 30, 2020 11:21
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants