Skip to content

chore: add console error logging in env.config.jsx template #254

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

Merged
merged 3 commits into from
May 8, 2025

Conversation

DawoudSheraz
Copy link
Contributor

Edly recently faced an issue with Teak sandbox in which env.config.jsx did not apply when both Indigo and Aspects plugins were enabled. Upon debugging, it was found that there was an issue in env.config.jsx related patches in Indigo which caused this. However, it was not apparent at first that this was the issue and it required debugging and re-building images to pinpoint the issue. While we are fixing the Indigo issue in overhangio/tutor-indigo#148, we are adding a small change in tutor-mfe to log the error in env.config.jsx if config fails to apply for any reason. This will make future debugging easier.

@DawoudSheraz DawoudSheraz force-pushed the dsheraz/add-env-config-logging branch from c417dcd to 9ab9e8e Compare May 6, 2025 12:41
@DawoudSheraz DawoudSheraz moved this from Pending Triage to In review in Tutor project management May 6, 2025
@DawoudSheraz DawoudSheraz requested a review from arbrandes May 6, 2025 12:42
Copy link
Collaborator

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

I usually object to console.logging in production code, but it seems plugin configuration and interaction will always be prone to errors. We can discuss a way to do conditional logging later, but for now I think this is worth it.

I do think we should log the error message as well, though.

@@ -43,7 +43,7 @@ async function setConfig () {
{%- endfor %}

{{- patch("mfe-env-config-runtime-final") }}
} catch { }
} catch (error) { console.error("env.config.jsx failed to apply.";}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're using a catch-all, it would be useful to log the error message as well, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I misunderstood console.error that it would automatically log the error, it is not the case.

@DawoudSheraz DawoudSheraz requested a review from arbrandes May 8, 2025 13:44
Copy link
Collaborator

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

Approved, with a small nit.

Co-authored-by: Adolfo R. Brandes <adolfo@axim.org>
@DawoudSheraz DawoudSheraz merged commit b9eaff0 into release May 8, 2025
2 checks passed
@DawoudSheraz DawoudSheraz deleted the dsheraz/add-env-config-logging branch May 8, 2025 16:27
@github-project-automation github-project-automation bot moved this from In review to Done in Tutor project management May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants