Use absolute directory paths everywhere #2962

colinrotherham · 2022-11-04T12:24:49Z

This PR addresses a few code review comments that have come up recently

Where are my files?
Having to use the variable process.cwd() to find where the project root

const pathToPackage = join(cwd(), configPaths.package, 'package.json')

Where is the src directory?
We suffix "govuk" into configPaths.src but (can) forget to join it to destination paths

const pathToSrc = join(configPaths.src, 'all.scss') // Hmm, no `govuk`?
const pathToDest = join(configPaths.package, 'govuk', 'all.scss')

Where is the actual src directory?
Remembering to add ../ to remove the "govuk" suffix

const pathToSrcForReal = join(cwd(), configPaths.src, '../')

This PR makes all our config paths absolute and removes the "govuk" suffix by default

Config paths

We currently used relative paths with trailing slashes

Before

Relative ish file paths

import configPaths from '../../config/paths.js'

configPaths.app     // relative 'app/'
configPaths.dist    // relative 'dist/'
configPaths.package // relative 'package/'
configPaths.public  // relative 'public/'

After (macOS, Linux)

Absolute file paths

import configPaths from '../../config/paths.js'

configPaths.app     // absolute '/path/to/project/govuk-frontend/app'
configPaths.dist    // absolute '/path/to/project/govuk-frontend/dist'
configPaths.package // absolute '/path/to/project/govuk-frontend/package'
configPaths.public  // absolute '/path/to/project/govuk-frontend/public'

After (Windows)

Absolute file paths

import configPaths from '../../config/paths.js'

configPaths.app     // absolute 'C:\\path\\to\\project\\govuk-frontend\\app'
configPaths.dist    // absolute 'C:\\path\\to\\project\\govuk-frontend\\dist'
configPaths.package // absolute 'C:\\path\\to\\project\\govuk-frontend\\package'
configPaths.public  // absolute 'C:\\path\\to\\project\\govuk-frontend\\public'

colinrotherham · 2022-11-04T12:38:49Z

Related code review comments that this PR helps address:

#2851 (review) from @36degrees

(I think it'd be neater if the .js and .scss files were handled by the existing copy-files task, but that looks like it's caught up in the fact that config.src is actually src/govuk so happy to revisit that later)

#2946 (comment) from @romaricpascal

We may have a slight naming issue there, as I thought configPaths.src was the src folder, while it's actually src/govuk.

colinrotherham · 2022-11-04T12:47:02Z

gulpfile.js

 */
 gulp.task('copy:assets', () => {
-  return gulp.src(`${configPaths.src}assets/**/*`)
+  return gulp.src(slash(join(configPaths.assets, '**/*')))


See commit 6e72471 regarding Gulp using its own glob library:

Note: Gulp globs use slash() to ensure Windows paths are flipped back to POSIX for compatibility

I did consider require('path/posix').normalize to flip Windows backslashes back to POSIX forward slashes but we'll continue to use slash() for this instead

colinrotherham · 2022-11-04T12:47:31Z

app/app.js

@@ -93,7 +93,7 @@ module.exports = async (options) => {
  app.use('/vendor/govuk_frontend_toolkit/', express.static('node_modules/govuk_frontend_toolkit/javascripts/govuk/'))
  app.use('/vendor/jquery/', express.static('node_modules/jquery/dist'))

-  app.use('/assets', express.static(join(configPaths.src, 'assets')))
+  app.use('/assets', express.static(configPaths.assets))


We used it enough times to add a new configPaths.assets

colinrotherham · 2022-11-04T12:50:59Z

lib/file-helper.js

@@ -28,7 +27,7 @@ const cache = global.cache || {}
 const getListing = async (directoryPath, pattern = '**/*', options = {}) => {
  const listing = await glob(pattern, {
    allowEmpty: true,
-    cwd: join(cwd(), directoryPath),
+    cwd: directoryPath,


We run our tooling from the project root so cwd() has been (mostly) stable for us

But since Node.js via node/nodemon can launch from any directory it's better that we use absolute paths instead like we do here

colinrotherham · 2022-11-04T12:53:47Z

src/govuk/settings/settings.test.js

@@ -39,7 +39,7 @@ describe('The settings layer', () => {

  describe('Sass documentation', () => {
    it('associates everything with a "settings" group', async () => {
-      const docs = await sassdoc.parse(join(configPaths.src, 'settings', '*.scss'))
+      const docs = await sassdoc.parse(join(configPaths.src, 'govuk/settings/**/*.scss'))


You'll still see us using / forward slashes within join() as @romaricpascal pointed out that normalize() is also run behind the scenes 🙌

https://nodejs.org/api/path.html#pathjoinpaths

The path.join() method joins all given path segments together using the platform-specific separator as a delimiter, then normalizes the resulting path.

colinrotherham · 2022-11-04T12:59:15Z

tasks/clean.js


 const { destination } = require('./task-arguments.js')
+const cleanPath = slash(destination)


Similar to Gulp's glob-parent not supporting Windows paths we know our "clean" task's del@6.1.1 package uses globby which also wants POSIX forward slashes 😭

colinrotherham · 2022-11-04T13:01:52Z

tasks/gulp/compile-assets.js

@@ -105,10 +105,10 @@ gulp.task('js:compile', async () => {

    // This is combined with destPath in gulp.dest()
    // so the files are output to the correct folders
-    const modulePath = slash(srcPath).replace(/^govuk/, '')
+    const modulePath = slash(srcPath).replace(/^govuk\/?/, '')


Tiny future bug fix here as we missed govuk/ → / trailing slashes

colinrotherham · 2022-11-04T13:03:12Z

tasks/gulp/watch.js

-      `${configPaths.fullPageExamples}**/*.scss`,
-      `!${configPaths.src}vendor/*`
+      `${slash(configPaths.src)}/govuk/**/**/*.scss`,
+      `${slash(configPaths.app)}/assets/scss/**/*.scss`,


Caught me out at first as this is different to configPaths.assets

colinrotherham · 2022-11-04T13:06:29Z

tasks/task-arguments.js

@@ -23,13 +23,13 @@ const destination = argv.destination || (destinations
  .filter(({ task }) => tasks.includes(task))[0]?.destination ?? 'public')

 const rootPath = dirname(__dirname)
-const destPath = resolve(rootPath, destination)
+const destPath = resolve(rootPath, slash(destination))


For the --destination flag we have tests for both forward slashes and backslashes

We're using slash() to remove Windows escaped paths \\ then resolve() to normalise to the platform default—Windows paths for Windows, POSIX paths for macOS/Linux

https://nodejs.org/api/path.html#pathresolvepaths

The resulting path is normalized and trailing slashes are removed unless the path is resolved to the root directory.

colinrotherham · 2022-11-04T13:08:01Z

tasks/task-arguments.js

-  // Normalise slashes (Windows) for gulp.src
-  destination: slash(relative(rootPath, destPath)),
+  // Absolute path to destination
+  destination: destPath,


We no longer return 'package', 'public', 'dist' but full absolute paths:

/path/to/project/govuk-frontend/package /path/to/project/govuk-frontend/public /path/to/project/govuk-frontend/dist

romaricpascal

That looks so much tidier 😍 Thanks for the tedious update across the project 🙌🏻

Ran the build on Windows and it was OK. Tests had 4 failures around the shapes of the slashes in the Clean task, though 😢 :

Summary of all failing tests
 FAIL  tasks/clean.unit.test.js
  ● Clean task › cleans destination "public"

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Array [
    -   "public/**/*",
    +   "public\\**\\*",
      ]

      32 |     jest.mock('./task-arguments.js', () => ({ destination: mockDestination }))
      33 |     const clean = await import('./clean.js')
    > 34 |     expect(clean.paths()).toEqual(paths)
         |                           ^
      35 |   })
      36 | })
      37 |

      at toEqual (tasks/clean.unit.test.js:34:27)

  ● Clean task › cleans destination "package"

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Array [
    -   "package/**/*",
    +   "package\\**\\*",
        "!package/",
        "!package/package.json",
        "!package/govuk-prototype-kit.config.json",
        "!package/README.md",
      ]

      32 |     jest.mock('./task-arguments.js', () => ({ destination: mockDestination }))
      33 |     const clean = await import('./clean.js')
    > 34 |     expect(clean.paths()).toEqual(paths)
         |                           ^
      35 |   })
      36 | })
      37 |

      at toEqual (tasks/clean.unit.test.js:34:27)

  ● Clean task › cleans destination "dist"

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Array [
    -   "dist/**/*",
    +   "dist\\**\\*",
      ]

      32 |     jest.mock('./task-arguments.js', () => ({ destination: mockDestination }))
      33 |     const clean = await import('./clean.js')
    > 34 |     expect(clean.paths()).toEqual(paths)
         |                           ^
      35 |   })
      36 | })
      37 |

      at toEqual (tasks/clean.unit.test.js:34:27)

  ● Clean task › cleans destination "custom/location/here"

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Array [
    -   "custom/location/here/**/*",
    +   "custom\\location\\here\\**\\*",
      ]

      32 |     jest.mock('./task-arguments.js', () => ({ destination: mockDestination }))
      33 |     const clean = await import('./clean.js')
    > 34 |     expect(clean.paths()).toEqual(paths)
         |                           ^
      35 |   })
      36 | })
      37 |

      at toEqual (tasks/clean.unit.test.js:34:27)


Test Suites: 1 failed, 78 passed, 79 total
Tests:       4 failed, 1421 passed, 1425 total
Snapshots:   33 passed, 33 total
Time:        220.556 s, estimated 231 s
Ran all test suites in 4 projects.

I think the issue is with the join on line 9 that turns back the path into a Windows path. As we're looking to create a glob, we can probably just use a template string.

Happy to run the tests again or pair to help fix it 😄

colinrotherham · 2022-11-07T12:50:48Z

I think the issue is with the join on line 9 that turns back the path into a Windows path. As we're looking to create a glob, we can probably just use a template string.

Ahh this is brilliant, thank you for being so thorough

I'll probably call slash() again as we do elsewhere to ensure POSIX for globs—can pair on that??

Reminds me I should rebase #2945 with this branch as it will have caught this. Thanks @romaricpascal

Unused since a940adc

Ensure absolute paths are used throughout

colinrotherham added the Frontend squad label Nov 4, 2022

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2962 November 4, 2022 12:25 Inactive

colinrotherham added the consistency label Nov 4, 2022

colinrotherham force-pushed the absolute-paths branch from 8fe3cd0 to 67a6e4f Compare November 4, 2022 12:29

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2962 November 4, 2022 12:29 Inactive

colinrotherham commented Nov 4, 2022

View reviewed changes

colinrotherham requested a review from a team November 4, 2022 13:08

colinrotherham mentioned this pull request Nov 4, 2022

Update copy:files to glob “src” not “src/govuk” #2964

Merged

romaricpascal self-assigned this Nov 7, 2022

romaricpascal requested changes Nov 7, 2022

View reviewed changes

colinrotherham added 4 commits November 7, 2022 13:34

Use absolute file paths everywhere

ba51721

Add new configPaths.assets path

1317b70

Remove unused configPaths.partials

511a2ef

Unused since a940adc

Remove govuk suffix from configPaths.src

b4f4e3c

Ensure absolute paths are used throughout

colinrotherham force-pushed the absolute-paths branch from 67a6e4f to b4f4e3c Compare November 7, 2022 13:34

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2962 November 7, 2022 13:34 Inactive

romaricpascal approved these changes Nov 7, 2022

View reviewed changes

colinrotherham merged commit f73b754 into main Nov 7, 2022

colinrotherham deleted the absolute-paths branch November 7, 2022 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use absolute directory paths everywhere #2962

Use absolute directory paths everywhere #2962

colinrotherham commented Nov 4, 2022 •

edited

Loading

colinrotherham commented Nov 4, 2022

colinrotherham Nov 4, 2022

colinrotherham Nov 4, 2022

colinrotherham Nov 4, 2022

colinrotherham Nov 4, 2022

colinrotherham Nov 4, 2022

colinrotherham Nov 4, 2022

colinrotherham Nov 4, 2022

colinrotherham Nov 4, 2022

colinrotherham Nov 4, 2022 •

edited

Loading

romaricpascal left a comment

colinrotherham commented Nov 7, 2022


		const { destination } = require('./task-arguments.js')
		const cleanPath = slash(destination)

Use absolute directory paths everywhere #2962

Use absolute directory paths everywhere #2962

Conversation

colinrotherham commented Nov 4, 2022 • edited Loading

Config paths

Before

After (macOS, Linux)

After (Windows)

colinrotherham commented Nov 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrotherham Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

romaricpascal left a comment

Choose a reason for hiding this comment

colinrotherham commented Nov 7, 2022

colinrotherham commented Nov 4, 2022 •

edited

Loading

colinrotherham Nov 4, 2022 •

edited

Loading