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

Schema for manifest validation is out of date #357

Closed
cjonesdoordash opened this issue Feb 9, 2023 · 6 comments
Closed

Schema for manifest validation is out of date #357

cjonesdoordash opened this issue Feb 9, 2023 · 6 comments
Assignees
Labels
dependencies Pull requests that update a dependency file good first issue Good for newcomers pinned

Comments

@cjonesdoordash
Copy link

The current JSON schema used for manifest validation only includes not-allowed and spanning for the incognito property where as the current v3 schema supports all of the following: https://developer.chrome.com/docs/extensions/mv3/manifest/incognito.

This causes builds to fail that don't skip manifest validation.

@tm1000 tm1000 self-assigned this Feb 10, 2023
@tm1000 tm1000 added good first issue Good for newcomers dependencies Pull requests that update a dependency file labels Feb 10, 2023
@tm1000
Copy link
Member

tm1000 commented Feb 14, 2023

Hi there

The schema is generated from the types at definitely typed. This will need to be added there: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/firefox-webext-browser/index.d.ts#L378

PR: DefinitelyTyped/DefinitelyTyped#64355

@here-nerd
Copy link
Contributor

here-nerd commented Mar 7, 2023

Hi there

The schema is generated from the types at definitely typed. This will need to be added there: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/firefox-webext-browser/index.d.ts#L378

PR: DefinitelyTyped/DefinitelyTyped#64355

@tm1000 Super thanks for taking this up. However, according to DefinitelyTyped/DefinitelyTyped#64355, it does not seem to be a quick fix. I have looked into bugzilla and I have found the exact issue https://bugzilla.mozilla.org/show_bug.cgi?id=1380812. It's open for 6 years and no assignee. May I propose, for practicality, we fix the issue here in this repo? What do you think?
Note: of course, I'd love to help Mozilla to fix the issue and it would benefit everybody. But a 6-years-open bug does not convince me that Mozilla would be taking quick decision (in review/ merge etc.). And I think we can do better than just waiting.

@tm1000
Copy link
Member

tm1000 commented Mar 7, 2023

@here-nerd Yeah I saw the response... not what I was hoping for.

We should fix it here. There should probably be some way for us to be able to add more types into the schema that is generated.

If you have some ideas feel free to PR or I will do it when I get some time. For now we will leave this issue open

With regards to Mozilla you'd have to actually implement split mode. Here we can just say its valid. Like the Mozilla spec states.

@tm1000 tm1000 added the pinned label Mar 7, 2023
@here-nerd
Copy link
Contributor

You're totally right about the split mode on Mozilla. It's simply not supported on Mozilla. Anyway, I still think Mozilla should have exposed generic types rather than exposing only its supported keys.

I want to help. I need a hint from you @tm1000 how manifest.schema.json is currently created. E.g. why can't we just simply add 'split' in the schema?
Anyway, there is also https://json.schemastore.org/chrome-manifest, which has the correct incognito key definition. How can we use that instead of the mozilla one?

@tm1000
Copy link
Member

tm1000 commented Mar 8, 2023

It's actually generated by this

"generate-manifest-schema": "typescript-json-schema ./src/plugin/tsconfig.json browser._manifest.WebExtensionManifest > ./src/plugin/manifest.schema.json"

Which turns the typescript into json. Then it uses avj to validate the schema against the manifest:

const validate = ajv.compile(manifestSchema);

I'm not opposed to using the chrome one. It should just be a simple file replace

Alternatively it could be generated from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/042141ce5f77f36df01c344ad09f32feda26c4fd/types/chrome/index.d.ts#L6908

Instead of the mozilla one

tm1000 added a commit that referenced this issue Mar 8, 2023
Updates the schema reference by merging chrome and mozilla types together for the manifest and then generating the manifest that way. Also add tests to test against manifest specific settings

#357
@tm1000
Copy link
Member

tm1000 commented Mar 8, 2023

@here-nerd Here's a PR that I think will solve all of this: #376

I also added tests

@tm1000 tm1000 closed this as completed Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file good first issue Good for newcomers pinned
Projects
None yet
Development

No branches or pull requests

3 participants