Skip to content

Conversation

@avivkeller
Copy link
Member

@avivkeller avivkeller 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
@avivkeller avivkeller requested a review from aduh95 April 12, 2024 12:48
@avivkeller
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
@avivkeller
Copy link
Member Author

The docs and tests should also be updated.

Will do!

@avivkeller
Copy link
Member Author

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

@avivkeller
Copy link
Member Author

avivkeller 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

@avivkeller
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.

@avivkeller
Copy link
Member Author

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

@LiviaMedeiros
Copy link
Member

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

@avivkeller
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!

@avivkeller
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

@avivkeller
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.

@avivkeller
Copy link
Member Author

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

@avivkeller
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?

@avivkeller
Copy link
Member Author

Ignore the reference, wrong issue

@himself65
Copy link
Member

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

@avivkeller
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

@avivkeller
Copy link
Member Author

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

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

Blocked by #52509

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

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

@avivkeller avivkeller closed this Apr 24, 2024
@avivkeller avivkeller 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