search

mapbox / mapbox-studio-classic

https://www.mapbox.com/mapbox-studio/
BSD 3-Clause "New" or "Revised" License
1.14k stars 229 forks source link

Fix path normalization to work with HTTP URLs containing host names. #1292

Closed zerebubuth closed 9 years ago

zerebubuth commented 9 years ago

See #1288 for context. Basically, dropping the host name causes "Invalid URL" failures deeper inside tilelive, but it's slightly complicated by the way the "url" library parses Windows file paths as if the drive letter were the host name.

zerebubuth commented 9 years ago

Hi @yhahn, sorry - I got distracted by some other stuff. Does this look good? Any changes you need/want me to make before it can get merged?

rmarianski commented 9 years ago

To resolve the error from this: https://github.com/mapzen/vector-datasource/issues/73 , I've had to also apply that same type of change to the req.query a couple of lines above too.

rmarianski commented 9 years ago

In case this is helpful:

https://github.com/zerebubuth/mapbox-studio/pull/1

zerebubuth commented 9 years ago

Hi @yhahn, @springmeyer - how does this PR look? What can I do to get this in shape for a merge?

Thanks!

springmeyer commented 9 years ago

@zerebubuth - thanks! I just got the master (aka mb-pages) branch green again on travis. Can you rebase and ensure your branch is fully passing?

zerebubuth commented 9 years ago

I've rebased, pushed, but unfortunately the master (mb-pages) branch fails with the following errors:

not ok 253 Error: Invalid JSON returned from Mapbox API: Unexpected token E
not ok 256 progress obj not created
not ok 258 mapid correctly generated
not ok 418 Error: Invalid JSON returned from Mapbox API: Unexpected token E

More details in the test log which seems to suggest that at least one of the tests requires AWS_ACCESS_KEY_ID to be set, but I don't have one of those (at least, one that would access anything owned by Mapbox).

ajashton commented 9 years ago

I tested out your branch since I'm running into the http url issue as well. Main problem solved but running test/middleware.test.js there is one failing:

⨯ normalizePaths
 not ok 4 should be equivalent
   ---
     operator: deepEqual
     expected: 'A Font Name'
     actual:   'nullA Font Name'
     at: Object.middleware.normalizePaths (/home/aj/Code/mapbox-studio-dev/lib/middleware.js:33:5)
   ...
zerebubuth commented 9 years ago

Thanks @ajashton! I've been able to fix that case, so hopefully it'll get a clean test pass now.

ajashton commented 9 years ago

Thanks @zerebubuth, tests are passing for me now. Merging.

springmeyer commented 9 years ago

Looks like windows tests on appveyor have been failing since this was merged. https://ci.appveyor.com/project/Mapbox/mapbox-studio/build/1.0.2030. Tracking at #1386.

screen shot 2015-06-15 at 4 33 53 pm

zerebubuth commented 9 years ago

@springmeyer I cannot find an actual error in either the x86 or the x64 builds - they appear to pass all the tests and simply report: Command exited with code 1. Perhaps this is something happening after the tests? Do you want to try reverting this PR and seeing if that fixes the issue?

wilhelmberg commented 9 years ago

I will look into it during the next days.

I would suggest, we change AppVeyor.yml like I did for osrm-backend. Then it should be easier to find problems and test locally.

AppVeyor.yml only contains settings specific AppVeyor: e.g. matrix, env vars, ...

A seperate appveyor-build.bat does the actual work.

build-local.bat replicates the settings of AppVeyor.yml to test locally.

springmeyer commented 9 years ago

@BergWerkGIS Thanks! Sounds good on moving script logic out.