Skip to content

Conversation

@ninachaubal
Copy link
Contributor

Resolves#634

@dmitrizagidulin
Copy link
Contributor

Thanks @ninachaubal! :)

Could you by any chance add a unit test or something, to that function?

@RubenVerborgh
Copy link
Contributor

@ninachaubal Thanks! However, interpolation is not the problem, but the extra slash is. No need to perform an actual resolve, since we know the first part will end in a slash and the second part is a constant without special path expressions. Let's add a test and then we can merge.

@ninachaubal
Copy link
ContributorAuthor

Hey, sorry I couldn't get to this sooner.

I agree that we don't need to perform an actual resolve here, but I'm not certain we can assume that first part will end in a slash. As far as I can tell, the first part will end in a slash if req.path ends in a slash and express doesn't make any guarantees about that.

Would it be reasonable to edit utils.getFullUri() to guarantee an ending slash? Alternatively, we could use something like url-join to ensure there aren't multiple slashes without doing a resolve.

On the tests, what's the best place to put them? Should I add a new file under test/unit for options or is there an existing test suite you would like me to add these to?

@RubenVerborgh
Copy link
Contributor

I'm not certain we can assume that first part will end in a slash.

Resolve is perfect then, thanks!

On the tests, what's the best place to put them?

Looks like test/integration/capability-discovery-test.js is the place to go.

@ninachaubalninachaubal changed the title Use url.resolve instead of string interpolationPrevent double slashes in the "service" link headerMay 13, 2018
@ninachaubal
Copy link
ContributorAuthor

@RubenVerborgh, could you take another look at this PR?

@RubenVerborghRubenVerborgh merged commit d1b590d into nodeSolidServer:developMay 20, 2018
@RubenVerborgh
Copy link
Contributor

Thanks!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ninachaubal@dmitrizagidulin@RubenVerborgh