Skip to content

Conversation

@connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Jun 30, 2021

Replaces all the LH_ROOT = '../../../' madness with const {LH_ROOT} = require('../../../root.js'). This does a few things:

  • Can just type LH_ROO+<enter> to auto-import this value to a file
  • Statically verified (no more doing directory calculus to know how many levels up to go) (is this only hard for me?)
  • Prepares the way for ☂️ Convert to ES modules #12689 (getting the directory of current file is no longer as simple as __dirname, so having the code in one place means less future boilerplate)

Beyond changing all instances of LH_ROOT = ..., I also grokked for lines that use __dirname to form a path that goes to the root, and changed them to use LH_ROOT.


I did not modify any files that use __dirname w/o crossing over the root directory (ex: report-assets.js does __dirname + '/renderer/util.js')–I have some ideas for that, maybe worth discussing here:

  1. could have a function getRelativePathEsm(importMeta, ...pathSegments) (usage: getRelativePathEsm(import.meta, 'somefile.json'))
  2. root.js could also export all the top-level directories (LH_CORE, LH_CLI, etc...)
  3. could just use LH_ROOT for everything, and have all paths used in code be relative to root

( the motivation is to avoid this code everywhere https://stackoverflow.com/a/50052194/2788187 )

@connorjclark connorjclark requested a review from a team as a code owner June 30, 2021 05:35
@connorjclark connorjclark requested review from adamraine and removed request for a team June 30, 2021 05:35
@google-cla google-cla bot added the cla: yes label Jun 30, 2021
@adamraine
Copy link
Contributor

Any chance we can use module aliases?

@connorjclark
Copy link
Collaborator Author

module aliases

WARNING: This module should not be used in other npm modules since it modifies the default require behavior! It is designed to be used for development of final projects i.e. web-sites, applications etc.

Copy link
Contributor

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

this looks good, but I'm not a huge fan of root.js, especially if it grows to have dirname/filename substitutes. I don't have a better suggestion than my pretty bad module-utils.js one, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants