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

TODO: Console and logLevel #13

Open
nathanjhood opened this issue Sep 27, 2024 · 4 comments
Open

TODO: Console and logLevel #13

nathanjhood opened this issue Sep 27, 2024 · 4 comments
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@nathanjhood
Copy link
Owner

nathanjhood commented Sep 27, 2024

Problem:

I started the project with a rough idea of having two, seperate/distinct possible logging modes; 'verbose' & 'debug'.

Since then, I came to conclusion that we should not really designate our own specifiers, nor expose some sort of API layer to the average consumer. Why? Because we can just wrap ESBuild's API instead.

To that end, our logging should really be migrated to best align with ESBuild's logging API.

The one part the appears to be missing from their side is having those logLevels indexed. There is no such "least logging" and "most logging" in a value sense, they are instead all just string literals grouped into a Type.

We can infer some reasonable indexing from supposed "depth" of each mode, and construct our own value system as follows:

const logLevelValue = (logLevel: ESBuild.LogLevel): number => {
    switch (logLevel) {
      case 'silent': {
        return 0;
      }
      case 'error': {
        return 1;
      }
      case 'warning': {
        return 2;
      }
      case 'info': {
        return 3;
      }
      case 'debug': {
        return 4;
      }
      case 'verbose': {
        return 5;
      }
      default: {
        throw new Error("No matching 'logLevel' case in switch statement");
      }
    }
  };

this could be much better, something like a Map would allow us to iterate through the keys... but, no big deal. I don't want to do this with an array because: I want to be deliberate about assigning values; switches are a low-cost operation; and, throwing on bad values can be much easier to debug... see, the default case matches everything besides the specified cases, doing all the required error-catching in one single line, which an array would not do

Then we can create a console (why not use the standard global one? see below) and simply check if(logLevelValue(options.logLevel) <= 4) console.debug(x) and so forth.

Why create a console in every function, instead of using the global?

NodeJS docs:

"Warning: The global console object's methods are neither consistently synchronous like the browser APIs they resemble, nor are they consistently asynchronous like all other Node.js streams. See the note on process I/O for more information."

By scoping a new console instance to the current function, we can be more sure that all sync/async activity is completed before the function returns.

Naming is important: we want to be able to strip all references to console in the code - a standard feature of ESBuild and of pkgroll. So, we need to over-write the globally-named console with a local new console.Console, which accepts the input parameter proc: NodeJS.Process to extract an stdout/stderr to write to.

The latterly part about scoped consoles has, for the most part, already been done. The exposition is important, since we're potentially running asynchronously multi-threaded child processes, which all have (the same) stdout/stderr access.

More pertinently, I need to revise the previous logging implementation to match ESBuilds', using our logging priority value switcher function from above.

It will be ideal that eventually, we should be able to use optional terminators (--) in between commands, so that consumers can do things like specify differing log levels for different commands.

Such as:

$ yarn esbuild-scripts -- test --logLevel=debug -- build --logLevel=info -- start --logLevel=warning

(...and so forth)

@nathanjhood
Copy link
Owner Author

nathanjhood commented Oct 3, 2024

Update:

  • esbuild-scripts start is also online and deployed for beta-testing
  • it runs silently; the CLI only gives a small clue that there is a server running currently; this is where the log level needs to be set
  • unfortunately it's a fine line between 'just useful stuff' and 'output overload'
  • also gives a chance to figure out how to connect with react-error-overlay

@nathanjhood
Copy link
Owner Author

  • added initial logging feature to start command
  • fine on first load, but too verbose on reload events
  • needs to use the same mechanism as build; leveraging the build option object's logLevel to control our own log level

@nathanjhood nathanjhood self-assigned this Oct 5, 2024
@nathanjhood nathanjhood added documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Oct 5, 2024
@nathanjhood
Copy link
Owner Author

nathanjhood commented Oct 8, 2024

This should morph into a wider topic, which should produce the following results:

  • add something like paths.esbuildConfig to the getClientPaths helper object;

We're already parsing options that would in theory be coming from the CLI (see above) and layering them over the fallbacks (in build and start), so that end-users have a theoretical zero-config experience; but, by adding this path helper, we can also allow end-users to have a root-level esbuild.config.*s in their React project, which could over-ride all of the fallback values.

In such a case, I'd suggest a priority as follows:

  1. CLI arguments - highest priority
  2. user-local config file - medium priority
  3. esbuild-scripts config defaults - lowest priority

This way, a user could benefit from

  1. having most of the required config handled for them by our defaults
  2. specifying parameters specific to their own project's needs in their own config file
  3. over-riding all of the above by passing the option as an arg when invoking the CLI, for one-off needs that don't persist (checking a behaviour, generating a one-off output file, etc)

To do this, the parsing of args - per the helper function(s) - should be occuring in each of the main scripts; build, start, and so on.

We can just use the ESBuild config object types to map a set of of process.parseArgs() tokens, so that for example, a config-object property of publicPath would become a valid CLI option of publicPath, which this case would be of type string; and so forth...

Thus, finally, given the following config:

// esbuild-scripts/src/config/esbuild/getBuildOptions.ts
const buildOptions = {
    publicPath: path.resolve(paths.appPublic), // points at users' '<project>/public' dir
}
// <project>/esbuild.config.ts
export default = {
    publicPath: path.resolve(__dirname, 'App_A', 'public'),
}
# cli
$ yarn esbuild-scripts build --publicPath="./App_B/public"

... the determined publicPath that esbuild sees would be "./App_B/public".

Lastly!

It would be great to then retain the "chained commands" from the initial ideas, and use 'options terminators' to separate args, like proposed since the beginning:

This would be much easier to do, once the scripts are dealing with their own arguments. The CLI should separate the separators, and pass each group of args to the 'positional arg' at the beginning of the group: build --publicPath="./publicA" -- start --publicPath="./publicB" --, and so forth... and that's pretty much the CLI's only job, then.

@nathanjhood nathanjhood changed the title Console and logLevel EDIT: Console and logLevel Oct 8, 2024
@nathanjhood nathanjhood changed the title EDIT: Console and logLevel TODO: Console and logLevel Oct 8, 2024
@nathanjhood
Copy link
Owner Author

Diverging massively from title, but the content is fine for me, here - the title should be expanded.

Found! Tailwind CLI has exactly what I was thinking of regarding atomically reading/writing to a single file...

/**
 * When rapidly saving files atomically a couple of situations can happen:
 * - The file is missing since the external program has deleted it by the time we've gotten around to reading it from the earlier save.
 * - The file is being written to by the external program by the time we're going to read it and is thus treated as busy because a lock is held.
 *
 * To work around this we retry reading the file a handful of times with a delay between each attempt
 *
 * @param {string} path
 * @param {number} tries
 * @returns {Promise<string | undefined>}
 * @throws {Error} If the file is still missing or busy after the specified number of tries
 */
export async function readFileWithRetries(path, tries = 5) {
  for (let n = 0; n <= tries; n++) {
    try {
      return await fs.promises.readFile(path, 'utf8')
    } catch (err) {
      if (n !== tries) {
        if (err.code === 'ENOENT' || err.code === 'EBUSY') {
          await new Promise((resolve) => setTimeout(resolve, 10))

          continue
        }
      }

      throw err
    }
  }
}

src/cli/build/utils.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant