Skip to content

Using already existing nstack plist to obtain appid and apikey #4

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nharbo
Copy link
Contributor

@nharbo nharbo commented Apr 20, 2022

to hide away the keys and simplify the setup

to hide away the keys and simplify the setup
@nharbo
Copy link
Contributor Author

nharbo commented Apr 21, 2022

The plist is used for fetching translations in the runscript, so why not use it for initialising NStack too? :)

Copy link

@jakobmygind jakobmygind left a comment

Choose a reason for hiding this comment

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

Nope, I don't like it, this is a hidden dependency, all keys should be injected in the app entry point. Maybe if you read the values from the plist file and use those values, this could be fine

@nharbo nharbo requested a review from jakobmygind April 25, 2022 11:33
Comment on lines +45 to +53
var localizations: ObservableLocalizations = {
guard
let url = Bundle.main.url(forResource: "NStack", withExtension: "plist"),
let config = try? PropertyListDecoder().decode(NStackConfig.self, from: try Data(contentsOf: url))
else {
fatalError("Failed to initialize NStack")
}
return startNStackSDK(appId: config.appId, restAPIKey: config.apiKey)
}()
Copy link

@jakobmygind jakobmygind Apr 28, 2022

Choose a reason for hiding this comment

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

Hmm, I think the callsite should be as it was where NStack is started just with the ids, but there could be a convenience function that would get the keys from the plist the lines before, I think this might be nice 😊 This seems a little more messy than is necessary in this particular location which we would like to be very clear, since it's the entrypoint for everything imo

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.

2 participants