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

Inject DevTools Script in Browser Extension #2778

Merged

Conversation

noi5e
Copy link
Contributor

@noi5e noi5e commented Aug 6, 2022

This moves all of the code that was previously running as an inline string in devtools.inspectedWindow.eval() like so:
Screen Shot 2022-08-05 at 6 34 11 PM

And moves it to a new inject.ts file:
Screen Shot 2022-08-05 at 6 34 51 PM


Developer Tools Web Extension Planning Issue: #2127

@vercel
Copy link

vercel bot commented Aug 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Aug 9, 2022 at 0:12AM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Aug 9, 2022 at 0:12AM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 6, 2022
interface DocumentEventMap {
editorStateUpdate: CustomEvent;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the highlight CustomEvent. I'm unsure if a declare global is what I'm supposed to be doing here. @thegreatercurve

Copy link
Contributor

Choose a reason for hiding this comment

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

You can do, it will just get referenced in this module scope either way.

const data = {lexicalKey: message.lexicalKey};
const detail = IS_FIREFOX
? // eslint-disable-next-line no-undef
cloneInto(data, document.defaultView) // see https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Sharing_objects_with_page_scripts#cloneinto
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only TS error I couldn't find a way to handle. This is a Firefox-only browser extension API.

There's a definition written in this package, should I just copy it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I just went ahead and wrote a cloneInto type. After all, it's just deep cloning the same object.

But now I get this error:
Screen Shot 2022-08-06 at 2 43 35 PM

Where to go from here? Declare the definition on the window?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the current type definition from types.ts:

export type cloneInto = (
  arg: {data: {lexicalKey: string}},
  arg2: WindowProxy,
) => {data: {lexicalKey: 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.

Something like this within content.ts?
Screen Shot 2022-08-06 at 4 40 39 PM

packages/lexical-devtools/src/inject/index.ts Outdated Show resolved Hide resolved
interface DocumentEventMap {
editorStateUpdate: CustomEvent;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can do, it will just get referenced in this module scope either way.

Copy link
Contributor

@thegreatercurve thegreatercurve left a comment

Choose a reason for hiding this comment

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

Looks good apart from the small type issues

@noi5e
Copy link
Contributor Author

noi5e commented Aug 9, 2022

@thegreatercurve My current fix for cloneInto being undefined is a little weird, you might want to take a look.

I ended up dropping a @ts-ignore to declare cloneInto on globalThis. This is following the lead of this third-party wrapper. Not sure what my alternative is at this point, there doesn't seem to be a DefinitelyTyped definition of this Firefox-only API.

Otherwise this is merge ready!

@thegreatercurve thegreatercurve merged commit 9593e31 into facebook:main Aug 9, 2022
@thegreatercurve
Copy link
Contributor

@thegreatercurve My current fix for cloneInto being undefined is a little weird, you might want to take a look.

I ended up dropping a @ts-ignore to declare cloneInto on globalThis. This is following the lead of this third-party wrapper. Not sure what my alternative is at this point, there doesn't seem to be a DefinitelyTyped definition of this Firefox-only API.

Otherwise this is merge ready!

@noi5e This seems fine to me. You're not polluting any global name space with this, and tbh, I'd probably just case it to any.

@noi5e noi5e deleted the inject-devtools-script branch August 9, 2022 18:53
thegreatercurve pushed a commit that referenced this pull request Nov 25, 2022
* move devtools code to injected script

* correct typescript errors

* write cloneInto type

* add injected script to web_accessible_resources

* fix highlighting bug

* wrapper function for cloneInto
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants