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

misc: get LH_ROOT via new file root.js #12724

Merged
merged 2 commits into from
Jun 30, 2021
Merged

misc: get LH_ROOT via new file root.js #12724

merged 2 commits into from
Jun 30, 2021

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
Member

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
Member

@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