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

fix docs.rs show features #209

Merged
merged 3 commits into from
Nov 14, 2022
Merged

fix docs.rs show features #209

merged 3 commits into from
Nov 14, 2022

Conversation

light4
Copy link
Contributor

@light4 light4 commented Nov 13, 2022

use crates.io to get deps info

I don't remember how it looks like before, any advice?

@Folyd
Copy link
Member

Folyd commented Nov 14, 2022

Thanks, @light4. I ever didn't aware the feature flags were broken. 😅

Copy link
Member

@Folyd Folyd left a comment

Choose a reason for hiding this comment

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

Overall, it looks good to me. Some improvement is needed, though.

In case you don't remember the old UI, here is the screenshot. It's up to you to keep the same or improve it.
image

manifest.jsonnet Outdated
@@ -46,7 +46,7 @@ else

local INDEX_MANAGER_FILES = ['core/storage.js', 'index-manager.js'];
json.addIcons(icons())
.addPermissions(['storage', 'unlimitedStorage'])
.addPermissions(['storage', 'unlimitedStorage', 'webRequest', 'webRequestBlocking', '*://crates.io/api/v1/crates/*'])
Copy link
Member

Choose a reason for hiding this comment

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

We use Manifest V3 already, after loading into Chrome, it said: webRequestBlocking' requires manifest version of 2 or lower. Actually, I think we can remove those permissions.

@@ -116,8 +116,9 @@ document.addEventListener("DOMContentLoaded", async () => {
async function enhanceFeatureFlagsMenu(menu) {
// Use rawCrateName to fetch the Cargo.toml, otherwise will get 404.
let cargoTomUrl = `https://docs.rs/crate/${rawCrateName}/${crateVersion}/source/Cargo.toml`;
let response = await fetch(cargoTomUrl);
let content = await response.text();
let crateAPIURL = `https://crates.io/api/v1/crates/${rawCrateName}/${crateVersion}`;
Copy link
Member

Choose a reason for hiding this comment

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

The crateVersion could be latest which is invalid for the API of crates.io. For example: https://crates.io/api/v1/crates/tokio/latest

We should ensure the enhanceFeatureFlagsMenu() function is called after crateVersion = parseCrateVersionFromDOM(), see line 97.

@Folyd
Copy link
Member

Folyd commented Nov 14, 2022

Also, don't forget to remove tests/script-lib.sepc.js.

@light4
Copy link
Contributor Author

light4 commented Nov 14, 2022

Updated, should I rebase to one commit?

Copy link
Member

@Folyd Folyd left a comment

Choose a reason for hiding this comment

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

Just some nitpicking.

@@ -46,7 +46,7 @@ else

local INDEX_MANAGER_FILES = ['core/storage.js', 'index-manager.js'];
json.addIcons(icons())
.addPermissions(['storage', 'unlimitedStorage'])
.addPermissions(['storage', 'unlimitedStorage', '*://crates.io/api/v1/crates/*'])
Copy link
Member

Choose a reason for hiding this comment

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

We can remove *://crates.io/api/v1/crates/* too.

Permission '://crates.io/api/v1/crates/' is unknown or URL pattern is malformed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, Firefox says TypeError: NetworkError when attempting to fetch resource.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Good to know. Thanks.

@@ -114,10 +114,14 @@ document.addEventListener("DOMContentLoaded", async () => {
});

async function enhanceFeatureFlagsMenu(menu) {
if (crateVersion === 'latest') {
crateVersion = parseCrateVersionFromDOM();
}
// Use rawCrateName to fetch the Cargo.toml, otherwise will get 404.
let cargoTomUrl = `https://docs.rs/crate/${rawCrateName}/${crateVersion}/source/Cargo.toml`;
Copy link
Member

Choose a reason for hiding this comment

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

The cargoTomUrl is useless now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for reminding me.

@Folyd
Copy link
Member

Folyd commented Nov 14, 2022

Updated, should I rebase to one commit?

I'll squash it into one commit.

@Folyd
Copy link
Member

Folyd commented Nov 14, 2022

I'll merge it. We can fix the permission issue in the next PR.

@Folyd Folyd merged commit b578fd8 into huhu:master Nov 14, 2022
@light4 light4 deleted the fix_docsrs_features branch November 14, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants