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(install): fix the case that package scope contains tilde. #7049

Merged
merged 7 commits into from
Nov 15, 2023

Conversation

Hanaasagi
Copy link
Collaborator

What does this PR do?

Close #7045

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

I wrote automated tests

@Hanaasagi Hanaasagi changed the title fix(install): fix the case that package name contains tilde. fix(install): fix the case that package scope contains tilde. Nov 11, 2023
@Jarred-Sumner
Copy link
Collaborator

huh that is expected to work? interesting

Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

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

Actually, this code should check that the path name between @ and / is a valid URL component.

Something like this:

pub fn hasURLComponentThatNeedsEncoding(str: []const u8) bool {
        const do_not_escape_when_encoding_uri_component = comptime brk: {
            // encodeURIComponent uses this table:
            const chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" ++
                "abcdefghijklmnopqrstuvwxyz" ++
                "0123456789" ++
                "!'()*-._~";

            var bitset = std.bit_set.IntegerBitSet(std.math.maxInt(u8)).initFull();
            for (chars) |char| {
                bitset.unset(@as(u8, char));
            }

            break :brk bitset;
        };

        for (str) |char| {
            if (do_not_escape_when_encoding_uri_component.isSet(char)) {
                return true;
            }
        }

        return false;
    }

npm uses encodeURIComponent

var slash_index: usize = 0;
for (target[1..], 0..) |c, i| {
switch (c) {
// Old packages may have capital letters
'A'...'Z', 'a'...'z', '0'...'9', '$', '-', '_', '.' => {},
'A'...'Z', 'a'...'z', '0'...'9', '-', '_', '.' => {},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I rewrite these switch branches to bitmap?

@dylan-conway dylan-conway merged commit 056d45c into oven-sh:main Nov 15, 2023
20 of 28 checks passed
@dylan-conway
Copy link
Member

Thank you!

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.

bun can't install @~3/svelte_mount in cli
3 participants