Skip to content

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Aug 22, 2022

While #383 only fixed the symptoms, this will fix the root issue that sometimes the logger is loaded before the OC variable is declared, So we need to wait for OC to become available.

Currently the behavior is nondeterministic because if the logger is loaded before the global OC variable was set the logger would never respect the configured logging level and stick with the default (warning even if you configured e.g. info on the back end).
So you might even loose some messages.

This PR will change the behavior to a deterministic one:
Until the OC variable is set and the level was not set manually, the logger will no use any logging level (equivalent to debug).
When the OC variable is set the logger will switch to that logging level.
So the logger is independent to the loading order of the scripts.

@susnux susnux added the bug Something isn't working label Aug 22, 2022
@Pytal
Copy link
Contributor

Pytal commented Jan 4, 2023

Encountered this error

Uncaught ReferenceError: OC is not defined
    at new LoggerBuilder (LoggerBuilder.js:35:1)
    at getLoggerBuilder (index.js:19:1)

when adding a new Vue component to core nextcloud/server#35637 and this seems to fix it! Thanks!

@Pytal Pytal requested a review from artonge January 4, 2023 01:46
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 but please adjust the version as @Pytal mentionned

this.detectLogLevel()
}

return this.factory(this.context)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is called regardless of the log level being read already or later after page load.

in other words, if this.context.level is changed at a later stage, it won't have an affect on the already created logger instance, will it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will, as the object is not deep copied but passed by reference so the logger and the logger builder share the same context (until the site it loaded).

I pushed two additional commits providing test cases for this scenario.
(I hope this is ok, normally I would open a new PR for a new feature but I think the tests belong to this)

susnux added 4 commits January 9, 2023 21:34
Sometimes the logger is loaded before the OC variable is declared,
so we need to wait for OC to become available.

Signed-off-by: Ferdinand Thiessen <[email protected]>
Also added mocks for the logger itself.

Signed-off-by: Ferdinand Thiessen <[email protected]>
@Pytal Pytal requested a review from ChristophWurst January 9, 2023 23:07
@ChristophWurst ChristophWurst merged commit dbb2412 into nextcloud-libraries:master Jan 10, 2023
@susnux susnux deleted the fix/initial_config branch January 10, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants