Skip to content

Conversation

@danxuliu
Copy link
Contributor

Follow up to #362

The global OC variable may be undefined depending on the initialization order, so optional chaining (OC?.config and OC?.debug) was added to prevent accessing fields on an undefined variable.

However, OC could be not only undefined, but also undeclared. Optional chaining can not be used on an undeclared root object, so it needs to be explicitly guarded against that to prevent a ReferenceError to be thrown.

The ReferenceError can be seen in the PDF viewer since the update of nextcloud/logger to 2.2.1 (note that it also happened after the update to 2.2.0).

The global "OC" variable may be undefined depending on the
initialization order, so optional chaining ("OC?.config" and
"OC?.debug") was added to prevent accessing fields on an undefined
variable.

However, "OC" could be not only undefined, but also undeclared. Optional
chaining can not be used on an undeclared root object, so it needs to be
explicitly guarded against that to prevent a ReferenceError to be
thrown.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

window.OC?.config?.loglevel might work as well, assuming window is always declared?

@susnux
Copy link
Contributor

susnux commented Aug 19, 2022

While this will fix the issue, but it will make the feature useless.
So if it is possible for apps to be loaded before the core scripts are loaded (OC) we should consider to postpone the configuration of the logging level until OC is available.

@danxuliu
Copy link
Contributor Author

window.OC?.config?.loglevel might work as well, assuming window is always declared?

Good point, I tried that and it seems to work as expected. However, I have never used TypeScript, so I do not know if something needs to be changed above in declare var OC: Nextcloud.v22.OC | Nextcloud.v23.OC | Nextcloud.v24.OC;

While this will fix the issue, but it will make the feature useless.

But this is essentially the same thing fixed by #362. Why is guarding against undeclared a problem but guarding against undefined fine?

So if it is possible for apps to be loaded before the core scripts are loaded (OC) we should consider to postpone the configuration of the logging level until OC is available.

I totally agree, that would be the best solution. Unfortunately I do not know how to do that :-)

@susnux
Copy link
Contributor

susnux commented Aug 19, 2022

But this is essentially the same thing fixed by #362. Why is guarding against undeclared a problem but guarding against undefined fine?

Yes it is, but I thought it was about CI builds where the OC variable is missing.
But if this happens with some apps also in production, then this fix will make it nondeterministic.

The logging level is only configured once, so sometimes the app will log as configured and sometimes falling back to the default even when something more verbose was configured.

@danxuliu
Copy link
Contributor Author

But this is essentially the same thing fixed by #362. Why is guarding against undeclared a problem but guarding against undefined fine?

Yes it is, but I thought it was about CI builds where the OC variable is missing.

Ah, ok :-)

But if this happens with some apps also in production, then this fix will make it nondeterministic.

The logging level is only configured once, so sometimes the app will log as configured and sometimes falling back to the default even when something more verbose was configured.

Without the fix they will fail to load at all in a nondeterministic way, which is even worse :-P

So I would apply the current fix for the time being until the proper fix can be developed. Of course if the proper fix can be ready soon (I mean, before the Nextcloud 25 RCs) then better go for the proper fix instead, but I do not know how to do it.

@susnux
Copy link
Contributor

susnux commented Aug 19, 2022

So I would apply the current fix for the time being until the proper fix can be developed. Of course if the proper fix can be ready soon (I mean, before the Nextcloud 25 RCs) then better go for the proper fix instead, but I do not know how to do it.

Currently I can only think of something like this as a solution:
master...susnux:nextcloud-logger:fix/initial_config

Not sure if this is the right way to go

@danxuliu
Copy link
Contributor Author

So I would apply the current fix for the time being until the proper fix can be developed. Of course if the proper fix can be ready soon (I mean, before the Nextcloud 25 RCs) then better go for the proper fix instead, but I do not know how to do it.

Currently I can only think of something like this as a solution: master...susnux:nextcloud-logger:fix/initial_config

Not sure if this is the right way to go

I have tested it (also replacing window.OC? with OC so it would fail if it was not declared) and it worked. However... I do not know if it would work in every case or if it happened to work with the specific call order of my setup but it would fail in other cases 🤷

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.

👍

@PVince81 PVince81 merged commit 8cd7b9c into master Aug 22, 2022
@delete-merged-branch delete-merged-branch bot deleted the do-not-use-optional-chaining-on-a-potentially-undeclared-root-object branch August 22, 2022 07:12
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.

5 participants