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

docs: browserView.webContents can be null #40569

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Juice10
Copy link

@Juice10 Juice10 commented Nov 20, 2023

Description of Change

null was introduced as a possibility for destroyed webContents by @mlaurencin in: #25411

This change updates the documentation (and hopefully the typescript types) to reflect that.

Fixes: #40567

Checklist

Release Notes

Notes: browserView.webContents types now correctly reflects that destroyed webContents can be set to null

Fixes: electron#40567

`null` was introduced as a possibility for destroyed webContents in this PR: electron#25411
Copy link

welcome bot commented Nov 20, 2023

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 20, 2023
@Juice10 Juice10 changed the title Added null as type for browserView.webContents docs: Added null as type for browserView.webContents Nov 20, 2023
@electron-cation electron-cation bot added documentation 📓 semver/patch backwards-compatible bug fixes labels Nov 20, 2023
@Juice10 Juice10 changed the title docs: Added null as type for browserView.webContents docs: added null as type for browserView.webContents Nov 20, 2023
@Juice10 Juice10 changed the title docs: added null as type for browserView.webContents docs: browserView.webContents can be null Nov 20, 2023
docs/api/browser-view.md Outdated Show resolved Hide resolved
@Juice10
Copy link
Author

Juice10 commented Nov 21, 2023

I need some help, tried to get and failed to get the project running & reproduce the CI issue locally.

I copied another part of the docs hoping this issue would disappear but it looks like it keeps happening regardless. If anyone can help out this would be great!

I'm getting the following issue:

$ npm run create-api-json && electron-typescript-definitions --api=electron-api.json && node spec/ts-smoke/runner.js

> electron@0.0.0-development create-api-json
> node script/create-api-json.js

An error occurred while processing: "/home/builduser/project/src/electron/docs/api/browser-view.md"
AssertionError: Property `view.webContents` _Experimental_ should have a declared type but it does not: expected null to not equal null
    at _headingToPropertyBlock (/home/builduser/project/src/electron/node_modules/@electron/docs-parser/dist/block-parsers.js:103:49)
    at Array.map (<anonymous>)
    at parsePropertyBlocks (/home/builduser/project/src/electron/node_modules/@electron/docs-parser/dist/block-parsers.js:142:63)
    at DocsParser.<anonymous> (/home/builduser/project/src/electron/node_modules/@electron/docs-parser/dist/DocsParser.js:174:85)
    at Generator.next (<anonymous>)
    at fulfilled (/home/builduser/project/src/electron/node_modules/@electron/docs-parser/dist/DocsParser.js:28:58)
error Command failed with exit code 1.

@@ -41,7 +41,10 @@ Objects created with `new BrowserView` have the following properties:

#### `view.webContents` _Experimental_

A [`WebContents`](web-contents.md) object owned by this view.
* `webContents` [WebContents](web-contents.md)
Copy link
Member

Choose a reason for hiding this comment

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

@Juice10 bullet points in properties aren't stylistically valid in our parsing logic - this should be removed. Bullet pointed items are parsed to refer to parameters passed to a function, of which a property would have none. Simply adding what you already did below:

A WebContents | null property that returns the WebContents owned by this view
or null if the contents are destroyed.

Fixes the issue. You can verify this by looking at the Archaeologist check in CI.

Copy link
Author

Choose a reason for hiding this comment

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

@codebytere, thanks for the suggestion, I really appreciate it! Unfortunately I'm still getting the same error. I've even tried a couple variations on what you suggested, but the error is still the same.
Any chance you could take another look? I'd be happy to tweak it again, but I'm a bit lost in what to do.

Copy link
Member

Choose a reason for hiding this comment

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

Did you try:

A WebContents | null property that returns the WebContents owned by this view
or null if the contents are destroyed.

Copy link
Author

@Juice10 Juice10 Nov 28, 2023

Choose a reason for hiding this comment

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

Replacing Returns WebContents | null with A WebContents | null fixed the issue!

I'm getting another issue now but it's no longer a docs parsing issue, but a typing issue so this is progress! Thanks for the help @codebytere

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Nov 21, 2023
docs/api/browser-view.md Outdated Show resolved Hide resolved
docs/api/browser-view.md Outdated Show resolved Hide resolved
docs/api/browser-view.md Outdated Show resolved Hide resolved
docs/api/browser-view.md Outdated Show resolved Hide resolved
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

thank for the fix (and for being patient through some snags!)

@codebytere
Copy link
Member

oh fun, one more issue - not with the docs though! Looks like your change surfaced an actual types issue in lib/browser/api/browser-window.ts - we need to add a check for browserView.webContents being null in BrowserWindow.fromBrowserView (and return null if it is).

@Juice10
Copy link
Author

Juice10 commented Dec 11, 2023

Thanks for catching that @codebytere! I've added a fix for the issue you mentioned 16e636d. Hopefully this fixes it.

@@ -91,6 +91,8 @@ BrowserWindow.fromWebContents = (webContents: WebContents) => {
};

BrowserWindow.fromBrowserView = (browserView: BrowserView) => {
const { webContents } = browserView;
if (!webContents) return null;
Copy link
Member

Choose a reason for hiding this comment

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

you'll need to update the docs for BrowserWindow.fromBrowserView to state that it can return null, too.

@nornagon
Copy link
Member

nornagon commented Feb 7, 2024

Looks like there are a bunch of type issues now in electron/spec/api-browser-view-spec.ts, which makes sense.

@zcbenz zcbenz added the wip ⚒ label Feb 8, 2024
@zcbenz
Copy link
Member

zcbenz commented Feb 8, 2024

Add wip label as the CI failures need to be solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📓 semver/patch backwards-compatible bug fixes wip ⚒
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: BrowserView.webContents can be null, but typescript types are incorrect
4 participants