feat: add a few utils #1

P0lip · 2019-06-20T03:35:29Z

Question:

Should we add tests for actual URIs?
https://github.com/stoplightio/json-ref-resolver/pull/60/files#r295356728
should toFSPath return a Windows-compatible path when executed under Windows?

TODO:

relative helper

marbemac · 2019-06-20T05:00:54Z

Should we add tests for actual URIs?

Yeah I think we should.

should toFSPath return a Windows-compatible path when executed under Windows?

What do you mean?

marbemac · 2019-06-20T05:12:01Z

Could you try yalcing this into graphite, json-ref-resolver, and studio if you have time today? And updating various usages in those projects to make sure these utils feel good? Thank you!

P0lip · 2019-06-20T10:44:06Z

should toFSPath return a Windows-compatible path when executed under Windows?

What I'm saying is whether the fn should generate the following path c:\foo\foo under Windows.

src/join.ts

P0lip · 2019-06-20T11:36:53Z

@marbemac
stoplightio/json-ref-resolver#61
https://github.com/stoplightio/graphite/pull/200
https://github.com/stoplightio/studio-internal/pull/318

billiegoose · 2019-06-20T14:04:31Z

What I'm saying is whether the fn should generate the following path c:\foo\foo under Windows.

No. Windows is perfectly happy with forward slash separators. The only part of Windows that doesn't like forward slash separators are old DOS shell commands that use forward slash to indicate CLI arguments (instead of dash like POSIX CLIs).

The only reason to use back slashes would be if users complain. There is no technical reason why we should convert to back slashes on certain platforms.

marbemac · 2019-06-20T20:11:28Z

What I'm saying is whether the fn should generate the following path c:\foo\foo under Windows.

Andddd a primary purpose of this path package is to normalize windows and posix where we can (in this case, to always use forward slashes). So that graphite lookup by uri works across os, etc.

marbemac · 2019-06-23T19:43:09Z

I'm still not sure why we don't just use path where possible (normalize, join, dirname, basename)? Maybe the codebase inside of path is not perfect, but it works... the only thing we need to do when using path is make sure that the result is using posix style slashes.

Besides that, I think we still need to cover a few things:

What happens when urls are passed into these functions? They don't all have to support urls, but we should at least document and know what happens. For example, does isAbsolute return true if a url is passed in right now? (we need it to).
Suggest adding relative.

In general, I'd suggest that for almost every method (perhaps not toFSPath) we simply:

check if is URL (for join, check if first part is url)
IF url, use appropriate urijs method
IF not url, use path

Basically, we need to:

support urls, posix style file paths, and windows style file paths.
normalize file paths to forward slashes

I think that will cover all of our use cases?

billiegoose · 2019-06-24T16:04:43Z

the only thing we need to do when using path is make sure that the result is using posix style slashes.

Unfortunately @marbemac, that's simply not true.

At a minimum we should stop using path and change our dependency to use path-browserify v1.0.0 so that we aren't depending on an unknown codebase that changes at the whim of bundlers and Electron versions and behaves completely differently depending on the environment.

In an ideal we'd use our own parser so we have full control and can understand what the library is doing, as I suggest in #2.

Edit: The reason that the minimum is OK is because at least then we have predictable behavior. I.e. the same code paths will be running whether the environment is Webpack, Jest, Node on Windows, or Node on Mac. Developers will be able to do things like, import { sep } from '@stoplight/path' and not be introducing bugs that only affect certain platforms.

billiegoose · 2019-06-24T16:05:21Z

In fact I feel so strongly about the "minimum" I'm going to change that right now and push it to this PR hold on.

P0lip · 2019-06-24T16:06:23Z

@wmhilton hold on with pushing to this branch. I'm about to push a commit.

billiegoose · 2019-06-24T16:07:10Z

ok. It shouldn't have any merge conflicts though, just swapping out from 'path' for from 'path-browserify'

billiegoose · 2019-06-24T16:22:02Z

I'm gonna re-run tests on Windows, make sure nothing is amiss

billiegoose · 2019-06-24T16:25:31Z

Tests pass on Windows for what that's worth:

billiegoose

Left a couple suggestions

billiegoose · 2019-06-24T16:38:56Z

src/__tests__/join.spec.ts

+  });
+
+  it('treats URLs as file URIs', () => {
+    expect(join('https://foo.test', 'com', 'baz')).toEqual('/com/baz');


Suggested change

expect(join('https://foo.test', 'com', 'baz')).toEqual('/com/baz');

expect(join('https://foo.test', 'com', 'baz')).toEqual('https://foo.test/com/baz');

treats URLs as file URIs

Yeah in general I think the premise of the test is wrong. URLs should be treated as URLs. File paths as file paths.

Should be able to pass the following in (prob not comprehensive, but ya get the idea):

join('https://foo.test', 'com', 'baz')

join('https://foo.com/pets', '..', 'users', 123)

join('/foo/bar', '..', 'baz')

join('c:/foo/bar', '..', 'baz')

join('c:/foo\bar', '..', 'baz')

src/__tests__/normalize.spec.ts

src/__tests__/relative.spec.ts

src/__tests__/startsWithWindowsDrive.spec.ts

src/posixify.ts

marbemac

Closer!

src/isAbsolute.ts

marbemac · 2019-06-24T18:12:33Z

src/__tests__/relative.spec.ts

+import { relative } from '../';
+
+describe('relative', () => {
+  it('handles URLs', () => {


Should also add some tests to make sure it handles the various file path cases (posix, windows, forward slash, mixed slash, etc).

marbemac · 2019-06-24T18:14:55Z

src/__tests__/join.spec.ts

+  });
+
+  it('treats URLs as file URIs', () => {
+    expect(join('https://foo.test', 'com', 'baz')).toEqual('/com/baz');


treats URLs as file URIs

Yeah in general I think the premise of the test is wrong. URLs should be treated as URLs. File paths as file paths.

Should be able to pass the following in (prob not comprehensive, but ya get the idea):

join('https://foo.test', 'com', 'baz')

join('https://foo.com/pets', '..', 'users', 123)

join('/foo/bar', '..', 'baz')

join('c:/foo/bar', '..', 'baz')

join('c:/foo\bar', '..', 'baz')

src/__tests__/isAbsolute.spec.ts

P0lip · 2019-06-26T20:44:11Z

src/__tests__/relative.spec.ts

+  });
+
+  it('handles Windows URIs', () => {
+    expect(relative('c:\\test\\baz', 'C:\\test\\foo')).toEqual('../../../C:/test/foo');


@marbemac weird things happening here.

looking much better - will's going to finish up the review!

billiegoose · 2019-06-30T04:24:17Z

@P0lip can I humbly suggest closing this PR in favor of #2 ?

P0lip · 2019-06-30T08:35:35Z

Closing favor of #2

feat: add a few utils

846707d

P0lip requested review from billiegoose and marbemac June 20, 2019 03:35

P0lip commented Jun 20, 2019

View reviewed changes

src/join.ts Outdated Show resolved Hide resolved

P0lip added 2 commits June 20, 2019 19:04

feat: use more robust join fn

e610480

feat: dirname

d12725a

P0lip force-pushed the feat/utils branch from d1c9d89 to d12725a Compare June 20, 2019 11:06

P0lip added 4 commits June 20, 2019 19:13

feat: re-export extname

7379f08

feat: re-export sep and resolve

93f3af6

feat: re-export sep, relative and resolve

d1b01f7

feat: basename

6522d19

feat: improve normalize

1a07dc8

refactor: prefer NodeJS path fns

f26feda

P0lip and others added 2 commits June 24, 2019 18:11

refactor: urijs

95bc58d

fix: prefer 'path-browserify' over unspecified implementation

5dc4e1a

chore: simplify normalize

8676046

billiegoose suggested changes Jun 24, 2019

View reviewed changes

marbemac previously requested changes Jun 24, 2019

View reviewed changes

P0lip added 5 commits June 25, 2019 21:15

test: more tests

d8ea7e6

fix: url parts joining

b923320

refactor: use isURL util in isAbsolute

741589d

chore: rename posixify to urlify

07bb2c7

test: relative

696022f

P0lip force-pushed the feat/utils branch from 610962b to 696022f Compare June 26, 2019 08:13

feat: add normalization to the mix

40560a4

P0lip commented Jun 26, 2019

View reviewed changes

marbemac requested review from marbemac and removed request for marbemac June 28, 2019 22:18

billiegoose mentioned this pull request Jun 30, 2019

feat: initial parser #2

Merged

P0lip closed this Jun 30, 2019

	expect(join('https://foo.test', 'com', 'baz')).toEqual('/com/baz');
	expect(join('https://foo.test', 'com', 'baz')).toEqual('https://foo.test/com/baz');

feat: add a few utils #1

feat: add a few utils #1

Uh oh!

Conversation

P0lip commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marbemac commented Jun 20, 2019

Uh oh!

marbemac commented Jun 20, 2019

Uh oh!

P0lip commented Jun 20, 2019

Uh oh!

Uh oh!

P0lip commented Jun 20, 2019

Uh oh!

billiegoose commented Jun 20, 2019

Uh oh!

marbemac commented Jun 20, 2019

Uh oh!

marbemac commented Jun 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

billiegoose commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

billiegoose commented Jun 24, 2019

Uh oh!

P0lip commented Jun 24, 2019

Uh oh!

billiegoose commented Jun 24, 2019

Uh oh!

billiegoose commented Jun 24, 2019

Uh oh!

billiegoose commented Jun 24, 2019

Uh oh!

billiegoose left a comment

Choose a reason for hiding this comment

Uh oh!

billiegoose Jun 24, 2019

Choose a reason for hiding this comment

Uh oh!

marbemac Jun 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marbemac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marbemac Jun 24, 2019

Choose a reason for hiding this comment

Uh oh!

marbemac Jun 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

P0lip Jun 26, 2019

Choose a reason for hiding this comment

Uh oh!

billiegoose commented Jun 30, 2019

Uh oh!

P0lip commented Jun 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

P0lip commented Jun 20, 2019 •

edited

Loading

marbemac commented Jun 23, 2019 •

edited

Loading

billiegoose commented Jun 24, 2019 •

edited

Loading