Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Clean up utils and use external url functions #202

Merged
merged 10 commits into from
Jul 26, 2016
Merged

Conversation

blink1073
Copy link
Contributor

@blink1073 blink1073 commented Jul 23, 2016

I could not find a suitable ajax library that has the features we are using.

cf jupyterlab/jupyterlab#446

@@ -9,7 +9,8 @@
"path-posix": "^1.0.0",
"phosphor-disposable": "^1.0.5",
"phosphor-signaling": "^1.2.0",
"requirejs": "^2.2.0"
"requirejs": "^2.2.0",
"url": "^0.11.0"
Copy link
Member

@jasongrout jasongrout Jul 24, 2016

Choose a reason for hiding this comment

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

FYI, I noticed that both url and the path module we are using are missing upstream bugfixes from npm (cf. defunctzombie/node-url#24). They may be better than if we tried to write something from scratch, though, so +1 to not immediately trying to write our own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually not using url, it should have been url-join, since url didn't offer anything over posix-path. Agreed that we don't want to have our own path implementation.

@blink1073
Copy link
Contributor Author

Note: I am going to use url.resolve for of urlPathJoin and urlJoinEncode, since it handles both cases nicely.

@blink1073 blink1073 changed the title Clean up utils and use external url join function [WIP] Clean up utils and use external url join function Jul 24, 2016
@blink1073 blink1073 changed the title [WIP] Clean up utils and use external url join function Clean up utils and use external url functions Jul 25, 2016
@blink1073
Copy link
Contributor Author

Ready.

@afshin
Copy link
Member

afshin commented Jul 26, 2016

👍

@afshin afshin merged commit fe678ed into jupyter:master Jul 26, 2016
@blink1073 blink1073 deleted the use-url branch March 12, 2017 11:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants