New synchronous JS function getLogger() + deprecated async function createLogger()

[jongpie]

New getLogger() function

Partially fixed #728 by adding a new function getLogger() in logger LWC that can be called synchronously, and deprecated the async function createLogger()

• This simplifies how developers use the logger LWC, and avoids some lingering JavaScript (JS) stack trace issues that occur in async functions
• The function createLogger() is still supported & functional, but it's now considered deprecated since it requires using await (which is no longer necessary with getLogger())

Note

This does not truly "fix" or improve the stack trace parsing used in async functions, and async functions will continue to have inaccurate function names reported in some browsers (typically, eval is listed as the function name). This is a ongoing challenge in JavaScript due to what info is/isn't available in stack traces in some browsers (most notably, webkit browsers like Chrome & Edge).

But with the new getLogger() function, Nebula Logger no longer requires the use of async / await - and often the only reason developers were using async / await was to be able to use Nebula Logger. In these situations, developers can eliminate async / await, and the resulting log entries will have accurate JS function names.

Example: Old createLogger() Usage

Previously, JS developers had to use async, await, and truthy checks to ensure that Nebula Logger's LWC had finished being loaded. This resulted in more complexity for developers, who just want to be able to log some stuff 😢

import { createLogger } from 'c/logger';

export default class LoggerDemo extends LightningElement {
  logger;

  // Some lifecycle event (typically connectedCallback()) has to run async & await createLogger()
  async connectedCallback() {
    this.logger = await createLogger();
    this.logger.info('Hello, world');
    this.logger.saveLog();
  }

  // @wire functions run around the same time as connectedCallback(), but there's no guaranteed order
  // This result in some logging requiring truthy checks using the safe navigator "?.", and some loss of logging data could occur
  @wire(returnSomeString)
  wiredReturnSomeString({ error, data }) {
    this.logger?.info('>>> logging inside a wire function, if the logger is loaded');
    if (data) {
      this.logger?.info('>>> wire function return value: ' + data);
    }
    if (error) {
      this.logger?.error('>>> wire function error: ' + JSON.stringify(error));
    }
  }
}

Example: New getLogger() Usage

Now, await is no longer needed, and a logger instance can be immediately initialized & used. This results in less code, and happier developers 🥳

import { getLogger } from 'c/logger';

export default class LoggerDemo extends LightningElement {
  logger = getLogger();

  connectedCallback() {
    // Immediately use this.logger - no await, no truthy checks
    this.logger.info('Hello, world');
    this.logger.saveLog();
  }
}

  @wire(returnSomeString)
  wiredReturnSomeString({ error, data }) {
    // Immediately use this.logger - no await, no truthy checks
    this.logger.info('>>> logging inside a wire function');
    if (data) {
      this.logger.info('>>> wire function return value: ' + data);
    }
    if (error) {
      this.logger.error('>>> wire function error: ' + JSON.stringify(error));
    }
  }
}

New exception() function

Resolved #763 by adding a JS function equivalent to the Apex method Logger.exception(). Both of these do 3 things in 1 line of code:

1. Log an exception
2. Save the log
3. Rethrow the exception

Previously in JS, this would have been 3 lines of code:

this.logger.error('An error occurred').setExceptionDetails(someJSError);
this.logger.saveLog();
throw someJSError;

Now, 1 line of code provides the same functionality:

this.logger.exception('An error occurred', someJSError);

More Improvements for JS Stack Trace Parsing

Fixed #776 by updating logic in loggerStackTrace.js to better handle parsing when lightning debug mode is disabled
Previously, stack traces worked when debug mode was enabled, but was still inaccurate in some browsers when debug mode was off due to some subtle differences in the generated stack traces.

[jamessimone]

• logger.test.js is a bit of a mess right now - it now contains similar test functions for all 3 supported approaches (listed below). I might eventually split this into separate test files later.

One possibility that might help to clean things up in that test class such that you don't need to repeat yourself so much would be using parameterized tests:

const getMarkupLogger = async () => {
  const logger = createElement('c-logger', { is: Logger });
  document.body.appendChild(logger);
  await flushPromises();
  return logger;
};

// and then in a describe block
  it.each([[createLogger], [getLogger], [getMarkupLogger]])('returns user settings', async loggingFunction => {
    getSettings.mockResolvedValue({ ...MOCK_GET_SETTINGS });
    const logger = await loggingFunction();

    const userSettings = logger.getUserSettings();

    expect(userSettings.defaultSaveMethod).toEqual('EVENT_BUS');
    expect(userSettings.isEnabled).toEqual(true);
    expect(userSettings.isConsoleLoggingEnabled).toEqual(false);
    expect(userSettings.isLightningLoggerEnabled).toEqual(false);
  });

In this example, each inner list passed to .each can have any number of arguments that are then passed to the second function, a la:

 it.each([
    [createLogger, 'createLogger'],
    [getLogger, 'getLogger deprecated'],
    [getMarkupLogger, 'markup logger']
  ])('returns user settings', async (loggingFunction, text) => {
 // ...
})

What do you think? I will continue reviewing, but that might really help to cut down on the duplication in that file

[jamessimone]

if you want, you can slightly simplify this to:

    /* eslint-disable no-await-in-loop */
    this.#taskQueue.forEach(async task => {
      task.output = await task.actionFunction(...task.actionArguments);
      processedTasks.push(task);
    });
    this.#taskQueue = [];

[jongpie]

Oooh yeah, I like this, it seems much easier to read. For some reason, I thought I was having an issue with using await for synchronous functions, so I added that instanceof check, but seems unnecessary to use it.

And I typically do use forEach() / I generally don't use while loops, so I'm trying to remember if there was some reason I used the while loop in this case 🤔.... I think I was worried about situations where an async task is running & new sync tasks are enqueued while async is finishing. Something like...

• Some entries are added & saveLog() is called (async task)
• While saveLog() is running, user takes some action, and another log entry is added to the task queue (before saveLog() finishes)

In that situation, would the forEach() function iterate over the newly-added task? I think I went with a while loop to try to handle that situation - my thought was that the condition while (this.#taskQueue.length > 0) would run at the end of each loop, and pick up any newly-enqueued tasks. I was worried forEach() would only iterate over the values that were present in the array at the start of the iteration - but if forEach() works just as well, I think your version is much more concise & easier to read.

[jamessimone]

I think you're right in saying that forEach() would not iterate over a newly added task, but I guess I thought the #isProcessing check would already prevent something from being added to the queue. Maybe a test for this is in order?

[jongpie]

Yeah, I think a test would be great - one of my worries with all of this is that some tasks will get skipped/accidentally dropped, resulting in data loss. I'll try to add a test - if you have any thoughts on how test for this, let me know.

[jamessimone]

@jongpie I left a few comments that I'd be curious to hear your thoughts on, especially the one for cleaning up logger.test.js

Overall this is looking good! Hopefully we can chat about all of this today

[jongpie]

• logger.test.js is a bit of a mess right now - it now contains similar test functions for all 3 supported approaches (listed below). I might eventually split this into separate test files later.

One possibility that might help to clean things up in that test class such that you don't need to repeat yourself so much would be using parameterized tests:

const getMarkupLogger = async () => {
  const logger = createElement('c-logger', { is: Logger });
  document.body.appendChild(logger);
  await flushPromises();
  return logger;
};

// and then in a describe block
  it.each([[createLogger], [getLogger], [getMarkupLogger]])('returns user settings', async loggingFunction => {
    getSettings.mockResolvedValue({ ...MOCK_GET_SETTINGS });
    const logger = await loggingFunction();

    const userSettings = logger.getUserSettings();

    expect(userSettings.defaultSaveMethod).toEqual('EVENT_BUS');
    expect(userSettings.isEnabled).toEqual(true);
    expect(userSettings.isConsoleLoggingEnabled).toEqual(false);
    expect(userSettings.isLightningLoggerEnabled).toEqual(false);
  });

In this example, each inner list passed to .each can have any number of arguments that are then passed to the second function, a la:

 it.each([
    [createLogger, 'createLogger'],
    [getLogger, 'getLogger deprecated'],
    [getMarkupLogger, 'markup logger']
  ])('returns user settings', async (loggingFunction, text) => {
 // ...
})

What do you think? I will continue reviewing, but that might really help to cut down on the duplication in that file

👀 Ooooh, yeah, I really like this idea, it's beautiful - I think this sounds like a much better approach. Before this PR, it already felt bad having two versions of the tests (one for createLogger() import and one for the old markup approach) - and now that this PR adds a third version, it's definitely been challenging to try to add consistent tests for all 3 approaches. In fact, I just gave up on consistency towards the end of the changes in this PR - the new tests I added for console function calls were only added for the getLogger() approach, even though I've typically been adding equivalent (but nearly identical) tests for the 2 other approaches. But it was becoming very tedious, so I think the current approach of duplication is... not fun. F--, would not recommend 👎

With all that said, I might hold off on making this change right now, and instead do it in another PR in the coming weeks. There are several things related to Jest tests that I'd like to revisit:

• I've really let the LWC test coverage for the whole repo slip over the last few months. Despite code coverage being a terrible thing for companies to use as a metric, I know in this case that the drop is because I haven't been putting as much effort into some of the Jest tests as I should be, and there's some functionality that I'd like to ensure is tested properly
• Some of the Jest tests haven't been updated in a while, so there's still some old test patterns being used that I'd like to cleanup (I was so young & naive with how I did some of the older tests 😅 ).
• I'd like to also have all the Jest tests run twice - once with a namespace prefix, and once without. A couple of the LWCs do test this already (e.g., c/logEntrEventStream), but the approaches used aren't consistent, and not all of the LWCs do it (e.g., c/loggerSettings doesn't, but should since LoggerSettings__c and its fields have a namespace prefix in the managed package).
  • Using logger.test.js as an example: I feel like there's probably some way to combine your suggestion of it.each() to have it tests all 3 approaches, but do it twice - once with a namespace, and once without (6 total variations of the tests would run)

I think I'll make another issue to cover all of the Jest stuff to improve in the repo (including using your approach above with it.each() in logger.test.js). Let me know if that approach makes sense

[jamessimone]

That makes perfect sense!

[codecov]

Codecov Report

Attention: Patch coverage is 94.30894% with 7 lines in your changes missing coverage. Please review.

Project coverage is 92.82%. Comparing base (cfdae6b) to head (f43a62b).

Files with missing lines	Patch %	Lines
...ore/main/logger-engine/lwc/logger/loggerService.js	90.00%	6 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #775      +/-   ##
==========================================
+ Coverage   92.25%   92.82%   +0.56%     
==========================================
  Files          74       75       +1     
  Lines        7219     7203      -16     
  Branches      199      190       -9     
==========================================
+ Hits         6660     6686      +26     
+ Misses        531      498      -33     
+ Partials       28       19       -9     

Flag	Coverage Δ
Apex	94.24% <100.00%> (ø)
LWC	86.31% <94.21%> (+3.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.