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

Implementation of configurable environment[stg, prod] #58

Merged
merged 13 commits into from
Dec 12, 2023

Conversation

pritam-bs
Copy link

@pritam-bs pritam-bs commented Oct 30, 2023

  • Flutter SDK version up
  • update dependencies
  • make stg, prod env configurable
  • refactor nstack builder

- update dependencies
- make stg, prod env configurable
example/lib/nstack.dart Outdated Show resolved Hide resolved
lib/models/nstack_config.dart Outdated Show resolved Hide resolved
lib/src/nstack_builder.dart Outdated Show resolved Hide resolved
lib/src/nstack_builder.dart Outdated Show resolved Hide resolved
Comment on lines 167 to 170
const _config = NStackConfig(projectId: '$projectId', apiKey: '$apiKey',);
const _config = NStackConfig(projectId: '$projectId', apiKey: '$apiKey', env: '$env');
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might need to discuss this more, with this implementation you would need to rebuild the code if you want to switch environments, but what if someone wants to switch programmatically or using an env variable like dart-defines.

Copy link
Author

Choose a reason for hiding this comment

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

The stg environment is only intended for NStack developers not for the end users.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pritam-bs can we document this in the code then? I know it's mentioned in the NStack docs, we can leave a link to it for example

Copy link
Author

Choose a reason for hiding this comment

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

I will try to generate doc using the template system in future PRs.

@pritam-bs pritam-bs requested a review from nivisi November 8, 2023 06:48
Copy link
Contributor

@nivisi nivisi left a comment

Choose a reason for hiding this comment

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

This template generator seems cool, great that you found it 👍

example/lib/nstack.dart Outdated Show resolved Hide resolved
example/pubspec.lock Outdated Show resolved Hide resolved
lib/models/language_response.dart Outdated Show resolved Hide resolved
lib/models/language_response.dart Outdated Show resolved Hide resolved
lib/models/language_response.dart Outdated Show resolved Hide resolved
lib/src/nstack_builder.dart Outdated Show resolved Hide resolved
lib/src/nstack_builder.dart Outdated Show resolved Hide resolved
lib/src/nstack_builder.dart Outdated Show resolved Hide resolved
lib/src/nstack_builder.dart Outdated Show resolved Hide resolved
lib/src/nstack_builder.dart Outdated Show resolved Hide resolved
@pritam-bs pritam-bs requested a review from nivisi November 14, 2023 15:35
lib/models/language_response.dart Outdated Show resolved Hide resolved
lib/src/nstack_repository.dart Outdated Show resolved Hide resolved
lib/src/nstack_template.txt Outdated Show resolved Hide resolved
lib/src/nstack_builder.dart Outdated Show resolved Hide resolved
@pritam-bs pritam-bs requested a review from nivisi November 30, 2023 18:16
example/lib/nstack.dart Outdated Show resolved Hide resolved
example/lib/main.dart Show resolved Hide resolved
lib/sdk/nstack_sdk.dart Show resolved Hide resolved
lib/src/nstack_repository.dart Outdated Show resolved Hide resolved
Comment on lines 165 to 170
if (!_initializedNStack) {
_nstack
.appOpen(Localizations.localeOf(context),
platformOverride: widget.platformOverride)
.whenComplete(() => widget.onComplete?.call());
_initializedNStack = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

@pritam-bs yeah this is because in Localizations.localeOf we're trying to depend on an inherited widget before the widget is add such subscriptions. We should add WidgetBinding.instance.addPostFrameCallback or get the localization in didChangeDependencies (be careful though as didChangeDependencies is executed quite often when a top level inherited widget changes (more here). Let's do either of these options

Copy link
Contributor

@nivisi nivisi left a comment

Choose a reason for hiding this comment

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

@pritam-bs I like how more compact and readable the code is now👍

Could you please install this linter package monstarlab_lints to ensure we maintain the needed code style?

lib/models/nstack_config.dart Outdated Show resolved Hide resolved
lib/src/nstack_builder.dart Outdated Show resolved Hide resolved
lib/src/nstack_builder.dart Outdated Show resolved Hide resolved
lib/src/nstack_builder.dart Outdated Show resolved Hide resolved
lib/src/nstack_builder.dart Outdated Show resolved Hide resolved
- flutter sdk version up
- build_runner version up
- handle empty language response
- other changes
@pritam-bs pritam-bs requested a review from nivisi December 7, 2023 09:02
example/lib/nstack.dart Outdated Show resolved Hide resolved
lib/templates/nstack_template.txt Outdated Show resolved Hide resolved
lib/templates/nstack_template.txt Outdated Show resolved Hide resolved
lib/templates/nstack_template.txt Outdated Show resolved Hide resolved
lib/templates/nstack_template.txt Outdated Show resolved Hide resolved
@pritam-bs pritam-bs requested a review from nivisi December 8, 2023 17:37
Copy link
Contributor

@nivisi nivisi left a comment

Choose a reason for hiding this comment

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

LGTM

@hassan-saleh-ml please have a look as well

@pritam-bs pritam-bs requested a review from nivisi December 12, 2023 10:47
@pritam-bs pritam-bs merged commit 77092a5 into develop Dec 12, 2023
1 check passed
@pritam-bs pritam-bs deleted the feature/environment_config branch December 12, 2023 10:47
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.

3 participants