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

Fix URL file:// path handling on Windows #3584

Merged
merged 3 commits into from
May 5, 2023
Merged

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented May 5, 2023

This PR fixes two Windows regressions added in #3527 and #3372

URL file paths with drive letters D:/ confuse path.relative() and throw errors in await import():

Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only URLs with a scheme in: file and data are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'd:'

File URLs with absolute paths

On Windows, Node.js file:// URL paths look like:

// Example file URL string
const file = 'file:///D:/path/to/asset'

Converting URLs to paths incorrectly

But the property new URL(file).pathname includes a leading forward slash:

// Example file URL pathname ❌
const pathname = '/D:/path/to/asset' 

We hadn't realised this breaks path.relative() on Windows with paths joined and not resolved:

'..\\..\\..\\..\\D:\\path\\to\\project\\packages\\govuk-frontend\\src\\govuk\\components\\character-count\\_index.scss'

Converting URLs to paths correctly

After investigating this issue (also "Converting between URLs and file paths") we see that:

  1. Compatible cross-platform “path to file URL” changes must use pathToFileURL()
  2. Compatible cross-platform “file URL to path” changes must use fileURLToPath()
// Example file URL via fileURLToPath() ✅ 
const path = 'D:\\path\\to\\asset'

Compatible cross-platform “path to file URL” changes must use `pathToFileURL()`

```
Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only URLs with a scheme in: file and data are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'd:'
```
Compatible cross-platform “file URL to path” changes must use `fileURLToPath()`
@colinrotherham colinrotherham added the 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) label May 5, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3584 May 5, 2023 10:51 Inactive
@colinrotherham
Copy link
Contributor Author

colinrotherham commented May 5, 2023

Test runs on Windows must be run manually:
https://github.com/alphagov/govuk-frontend/actions/runs/4893403132

GitHub Actions UI to run on Windows

@colinrotherham colinrotherham requested a review from a team as a code owner May 5, 2023 11:16
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3584 May 5, 2023 11:16 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3584 May 5, 2023 11:57 Inactive
When a build is skipped (already in cache) we assume the global “npm get cache” location at `~/.npm` still has all the binaries we need such as eslint, stylelint, tsc etc

But Windows can’t find them unless `node_modules/.bin` is restored
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3584 May 5, 2023 12:14 Inactive
@@ -48,6 +48,9 @@ jobs:
- name: Checkout
uses: actions/checkout@v3

- name: Restore dependencies
uses: ./.github/workflows/actions/install-node
Copy link
Contributor Author

@colinrotherham colinrotherham May 5, 2023

Choose a reason for hiding this comment

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

Had to move this line here for Windows to find node_modules/.bin/tsc

Unlike our Linux runners it's not in the global “npm get cache” location at ~/.npm

> build:types
> tsc --build tsconfig.build.json

'tsc' is not recognized as an internal or external command,
operable program or batch file.

Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Cheers for catching that! Looks good to me ⛵

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation)
Projects
Development

Successfully merging this pull request may close these issues.

3 participants