Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support URL as cwd #201

Merged
merged 9 commits into from Jan 15, 2022
Merged

Support URL as cwd #201

merged 9 commits into from Jan 15, 2022

Conversation

fisker
Copy link
Collaborator

@fisker fisker commented Jan 12, 2022

I build url-or-path few months ago, it actually accept path, URL and URL.href, should we add test for URL.href too?

Fixes #176

@sindresorhus
Copy link
Owner

I would prefer not to introduce a dependency for this. I have worked very hard in the past year to reduce the amount of dependencies I use. In this case, it's really just cwd.startsWith('file://') ? fileURLToPath(cwd) : cwd.

@sindresorhus
Copy link
Owner

I think Node.js should expose a method for this. It's such a common need now that everything is starting to support URL. Maybe you wanna open an issue about it on the Node.js repo?

@sindresorhus
Copy link
Owner

path.from(string | URL) would be nice API.

@fisker
Copy link
Collaborator Author

fisker commented Jan 14, 2022

Removed dependency, and added test for file: string too.

@fisker
Copy link
Collaborator Author

fisker commented Jan 14, 2022

It's such a common need now that everything is starting to support URL. Maybe you wanna open an issue about it on the Node.js repo?

I think this is a temporary need. Once everything supports URL, convert to path won't be necessary. Anyway, nodejs/node#41521

@sindresorhus
Copy link
Owner

I think this is a temporary need. Once everything supports URL, convert to path won't be necessary. Anyway,

No, there will always be a need as your argument only works if you directly pass the input to a Node.js API. Many packages manipulate the paths somehow and dealing with path and not URL is much nicer then.

@sindresorhus sindresorhus merged commit 73c0aca into sindresorhus:main Jan 15, 2022
@fisker fisker deleted the cwd-url branch January 15, 2022 09:43
}

export interface GitignoreOptions {
readonly cwd?: string;
readonly cwd?: URL | string;
Copy link
Contributor

Choose a reason for hiding this comment

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

CC @sindresorhus

URL interface belongs to lib.dom.d.ts, so now we force TS users to inject compilerOptions.lib: ['dom'] to resolve URL as global. It breaks TS builds and formally this is a breaking change.

[build:es6   ] ../../node_modules/globby/index.d.ts(54,17): error TS2304: Cannot find name 'URL'.
[build:es6   ] ../../node_modules/globby/index.d.ts(58,17): error TS2304: Cannot find name 'URL'.
[build:es6   ] ../../node_modules/globby/index.d.ts(162,18): error TS2304: Cannot find name 'URL'.

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise it should be explicitly imported in index.d.ts. It does not belong to global by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sindresorhus
I really don't want to fix hundreds of our globby@^12-dependant projects all night )). Could you plz roll the patch #206?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sorry for breaking your projects, but I know nothing about the typing, I can't help.

antongolub added a commit to antongolub-forks/globby that referenced this pull request Jan 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept URL as options.cwd?
3 participants