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

lib, path: support file:/// urls in path functions #52497

Closed
wants to merge 7 commits into from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Apr 12, 2024

This PR adjusts the builtin path functions to support file:/// URLs.

Related to: #49273
Related to: #41521

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. labels Apr 12, 2024
lib/path.js Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev requested a review from aduh95 April 12, 2024 12:48
@RedYetiDev
Copy link
Member Author

Okay, now the linting should pass

@aduh95
Copy link
Contributor

aduh95 commented Apr 12, 2024

The docs and tests should also be updated.

@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 12, 2024
@RedYetiDev
Copy link
Member Author

The docs and tests should also be updated.

Will do!

@RedYetiDev
Copy link
Member Author

What semver will this be, so I can update the docs with the change version?

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Apr 12, 2024

I didn't modify the YAML specs, but I did update the docs. I will re-lint this and update the tests shortly

@RedYetiDev
Copy link
Member Author

Docs are ready, but the tests are kind of a mess, so once I figure out where to add them, I will.

@RedYetiDev
Copy link
Member Author

I'll check out the build issues when I get a chance

@LiviaMedeiros
Copy link
Contributor

LiviaMedeiros commented Apr 12, 2024

What semver will this be, so I can update the docs with the change version?

The version for YAML changelog must be REPLACEME, like this:

changes:
  - version: REPLACEME
    pr-url: https://github.com/nodejs/node/pull/52497
    description: Added `URL` support

Refs: https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#step-3-code

@RedYetiDev
Copy link
Member Author

What semver will this be, so I can update the docs with the change version?

The version for YAML changelog must be REPLACEME, like this:

changes:

  - version: REPLACEME

    pr-url: https://github.com/nodejs/node/pull/52497

    description: Added `URL` support

Will do, thank you!

@RedYetiDev
Copy link
Member Author

The checks have this issue:

TypeError: path.resolve is not a function
    at pathToFileURL (node:internal/url:1532:23)
    at evalScript (node:internal/process/execution:82:19)
    at node:internal/main/eval_string:51:3

I'll do some investigating

@RedYetiDev
Copy link
Member Author

The checks have this issue:

TypeError: path.resolve is not a function
    at pathToFileURL (node:internal/url:1532:23)
    at evalScript (node:internal/process/execution:82:19)
    at node:internal/main/eval_string:51:3

I'll do some investigating

I did some testing, and I'm not sure what's causing this. AFAICT the resolve function is defined

@himself65
Copy link
Member

The checks have this issue:

TypeError: path.resolve is not a function
    at pathToFileURL (node:internal/url:1532:23)
    at evalScript (node:internal/process/execution:82:19)
    at node:internal/main/eval_string:51:3

I'll do some investigating

I did some testing, and I'm not sure what's causing this. AFAICT the resolve function is defined

it seems like there is a circular dependency.

@RedYetiDev
Copy link
Member Author

:-/ okay, that puts some dents in the implementation. I'll sort it out tomorrow (EST).

@RedYetiDev
Copy link
Member Author

It is a circular dependency because path.resolve is used in the function we are trying to use in path.resolve. It's not worth it to re-implement the entire system just for this, so I'm stumped. Does anyone have any ideas?

@RedYetiDev
Copy link
Member Author

Ignore the reference, wrong issue

@himself65
Copy link
Member

use lazy load, you might found such code in other files

@RedYetiDev
Copy link
Member Author

I can try it, but I'm worried that (and this might be completely incorrect) once the lazyloading is complete, then it will throw a circular dependency errro

Co-authored-by: Livia Medeiros <livia@cirno.name>
@RedYetiDev
Copy link
Member Author

Temporarily moving to draft state, as changes in #52509 will be applied to this PR (if that one is merged)

@RedYetiDev RedYetiDev marked this pull request as draft April 15, 2024 13:57
@RedYetiDev RedYetiDev added the blocked PRs that are blocked by other issues or PRs. label Apr 22, 2024
@RedYetiDev
Copy link
Member Author

Blocked by #52509

@RedYetiDev RedYetiDev removed the blocked PRs that are blocked by other issues or PRs. label Apr 24, 2024
@RedYetiDev
Copy link
Member Author

Given the conflicts, I am closing this PR, and I will open a new one sometime this week.

@RedYetiDev RedYetiDev closed this Apr 24, 2024
@RedYetiDev RedYetiDev deleted the patch-2 branch April 24, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants