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

Add functionality to accept a JSON template string when initializing a RC server template #2520

Merged
merged 9 commits into from
Apr 4, 2024

Conversation

trekforever
Copy link

@trekforever trekforever commented Apr 2, 2024

Discussion

During testing, we commonly stored a template as JSON. It was then inconvenient to parse that into an object of a specific type (TemplateData) just to accommodate the type expected by the option.

This change updates initServerTemplate to also accept a JSON string, in addition to the existing ServerTemplateData.

Testing

Unit tested via npm test

API Changes

The InitServerTemplateOptions.template now accepts a union type ServerTemplateData|string instead of just ServerTemplateData

@trekforever trekforever self-assigned this Apr 2, 2024
src/remote-config/remote-config-api.ts Show resolved Hide resolved
src/remote-config/remote-config-api.ts Outdated Show resolved Hide resolved
} catch (e) {
throw new FirebaseRemoteConfigError(
'invalid-argument',
`Failed to parse the JSON string: ${options?.template}. ` + e
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the other errors we throw, we don't include the raw error (e) in the message. We should check w Lahiru if this is an intentional pattern, and if so, follow it here.

Nit: If we do include the error, it might be more readable to differentiate it from the other error message, something like "... Raw error: ${e}".

Copy link
Author

Choose a reason for hiding this comment

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

We don't include it for most errors, but we do have it for the createTemplateFromJSON:
https://github.com/firebase/firebase-admin-node/blob/master/src/remote-config/remote-config.ts#L163-L166
(most of this logic is consistent with that method). But will check with Lahiru when I request his review on this

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. I overlooked that. Makes sense. Shipit 🚢

src/remote-config/remote-config-api.ts Show resolved Hide resolved
Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM!

@trekforever trekforever merged commit 0aca056 into ssrc Apr 4, 2024
5 checks passed
@trekforever trekforever deleted the ssrc-json branch April 4, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants