Fix bug when data dir contains leading slash #1563
Conversation
When it contains leading slash, it creates url like "//data/..." which is interpretted as an absolute url.
| }); | ||
|
|
||
| it('ignores leading slash in dataDir', () => { | ||
| const router = createRouter('/data/', /*demoMode=*/false); |
There was a problem hiding this comment.
Shouldn't demoMode be true if these are tests for demoMode? Not sure it's necessary to test separately though given that this isn't really critical when in demo mode.
| ext: string, params?: URLSearchParams): string { | ||
|
|
||
| const url = new URL(`${window.location.origin}/${pathPrefix}${path}`); | ||
| const url = new URL(`${window.location.origin}`); |
There was a problem hiding this comment.
The URL wrapper is working pretty indirectly here, apparently by doing some implicit normalization when the properties are set (like adding a leading slash to the path). Can you add a comment to explain that?
| '/data/plugin/scalars/scalar'); | ||
| }); | ||
|
|
||
| it('ignores leading slash in dataDir', function() { |
There was a problem hiding this comment.
These tests logically are more testing createRouter() than pluginRoute(), and seem like either way they should be grouped with the existing test for "removes trailing slash from base route".
Can you also add an explicit test for the case where it adds a leading slash when there isn't one there? This is implicitly tested by the "removes trailing slash" test but it'd be better to have an explicit test. Or it could be merged with the test here as something like "ensures at least one leading slash in dataDir".
| '/data/plugin/scalars/scalar'); | ||
| }); | ||
|
|
||
| it('ignores leading slash in dataDir', function() { |
There was a problem hiding this comment.
Can we standardize the naming on "pathPrefix" as you used for createProdPath(), rather than "dataDir"? It's confusing that it's inconsistent between parts of the code and code vs tests, and "pathPrefix" both is clearer and matches the flag naming.
Nothing except these two files seems to reference dataDir, so I think just a find-replace would suffice.
nfelt
left a comment
nfelt
left a comment
There was a problem hiding this comment.
LGTM, thanks for the cleanup!
2 participants
When it contains leading slash, it creates url like "//data/..." which
is interpretted as an absolute url.