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 URI malformed issue for links with percent symbol #5

Merged
merged 1 commit into from
Nov 16, 2015
Merged

Fix URI malformed issue for links with percent symbol #5

merged 1 commit into from
Nov 16, 2015

Conversation

andrey-hohlov
Copy link
Contributor

@nmrugg
Copy link
Owner

nmrugg commented Oct 16, 2015

Thanks for the PR. But it looks like we want encodeURI() not encodeURIComponent(). Can you fix that?

@andrey-hohlov
Copy link
Contributor Author

Yes, we want encodeURI() - fixed

@nmrugg
Copy link
Owner

nmrugg commented Oct 16, 2015

Great. Can you squash the commits for me?

@andrey-hohlov
Copy link
Contributor Author

Done. Please check that everything I did right (not enough git expirience)

@nmrugg
Copy link
Owner

nmrugg commented Oct 16, 2015

Perfect. But I'm a little busy now. I'll try to check it once more over the weekend and then merge.

@nmrugg
Copy link
Owner

nmrugg commented Oct 19, 2015

I've been a little busy, and took me a bit to be able to sit down and think about this. Is it just me or does this now just undo the very thing it's supposed to be doing?

Correct me if I'm wrong, but it seems that the problem is that if we try to decodeURI() a string that has not been encodeURI()'ed, it may throw an error. So, it seems like what we should do is use a try/catch block to ignore a malformed (or non-encodeURI()'ed) string.

What do you think?

@andrey-hohlov
Copy link
Contributor Author

Yes, problem with not-encoded url strings in this case.

We always need decodeURI'ed link, right?
I think we can replace code with try/catch. I test it with that link and it's work:

try {
        link = decodeURI(String(link));
    } catch (err) {
        // do nothing
    }

But can be any more links with another problem? Or can be problem in next step in this function? By the way we can solve this problem and waiting for another bug reports if exist another type of "bad" links.

In current case "bad" link just ignored, hash added to links before that in document, and links after that.

@nmrugg
Copy link
Owner

nmrugg commented Nov 9, 2015

Sorry for the slow reply. I think the try/catch code you posted should work. I don't think there are other types of bad links. Do you want to update the PR?

@andrey-hohlov
Copy link
Contributor Author

Thank you for reply! PR updated.

nmrugg added a commit that referenced this pull request Nov 16, 2015
Fix URI malformed issue for links with percent symbol
@nmrugg nmrugg merged commit 5ff3d75 into nmrugg:master Nov 16, 2015
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