node icon indicating copy to clipboard operation
node copied to clipboard

node-api: preserve URL filenames without conversion

Open mertcanaltin opened this issue 6 months ago • 7 comments

allows us to process filenames that are already URLs by passing them unchanged

mertcanaltin avatar Jun 04 '25 19:06 mertcanaltin

Review requested:

  • [ ] @nodejs/node-api

nodejs-github-bot avatar Jun 04 '25 19:06 nodejs-github-bot

Codecov Report

:x: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 90.14%. Comparing base (641653b) to head (6b50c57). :warning: Report is 853 commits behind head on main.

Files with missing lines Patch % Lines
src/node_api.cc 40.00% 2 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58578      +/-   ##
==========================================
- Coverage   90.21%   90.14%   -0.07%     
==========================================
  Files         635      636       +1     
  Lines      187494   187895     +401     
  Branches    36838    36879      +41     
==========================================
+ Hits       169144   169375     +231     
- Misses      11145    11287     +142     
- Partials     7205     7233      +28     
Files with missing lines Coverage Δ
src/node_api.cc 75.91% <40.00%> (-0.26%) :arrow_down:

... and 63 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jun 04 '25 20:06 codecov[bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67361/

nodejs-github-bot avatar Jun 09 '25 13:06 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/67422/

nodejs-github-bot avatar Jun 12 '25 17:06 nodejs-github-bot

The CI failures are relevant:

duration_ms: 212.99
exitcode: 1
severity: fail
stack: |-
  node:assert:92
    throw new AssertionError(obj);
    ^

  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  + actual - expected

  + 'c:\\workspace\\node-test-binary-windows-native-suites\\node\\test\\node-api\\test_general\\build\\Release\\test_general.node'
  - 'file:///c:/workspace/node-test-binary-windows-native-suites/node/test/node-api/test_general/build/Release/test_general.node'

      at Object.<anonymous> (c:\workspace\node-test-binary-windows-native-suites\node\test\node-api\test_general\test.js:18:10)
      at Module._compile (node:internal/modules/cjs/loader:1733:14)
      at Object..js (node:internal/modules/cjs/loader:1898:10)
      at Module.load (node:internal/modules/cjs/loader:1468:32)
      at Module._load (node:internal/modules/cjs/loader:1285:12)
      at TracingChannel.traceSync (node:diagnostics_channel:322:14)
      at wrapModuleLoad (node:internal/modules/cjs/loader:235:24)
      at Module.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:152:5)
      at node:internal/main/run_main_module:33:47 {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: 'c:\\workspace\\node-test-binary-windows-native-suites\\node\\test\\node-api\\test_general\\build\\Release\\test_general.node',
    expected: 'file:///c:/workspace/node-test-binary-windows-native-suites/node/test/node-api/test_general/build/Release/test_general.node',
    operator: 'strictEqual'
  }

  Node.js v25.0.0-pre

legendecas avatar Jun 13 '25 15:06 legendecas

I opened an issue on WHATWG to discuss Windows file path handling in URL parsing

This discussion started from our PR here and ada-url PR https://github.com/ada-url/ada/pull/957. I wonder what WHATWG thinks about this behavior. https://github.com/whatwg/url/issues/873

mertcanaltin avatar Jun 13 '25 19:06 mertcanaltin

Blocked as per https://github.com/nodejs/node/pull/58578#issuecomment-2970754561.

lpinca avatar Jun 14 '25 14:06 lpinca