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

Clean Startup Class #1245

Merged
merged 8 commits into from
Jun 15, 2021
Merged

Clean Startup Class #1245

merged 8 commits into from
Jun 15, 2021

Conversation

hishamco
Copy link
Contributor

No description provided.

@ADefWebserver
Copy link
Member

This may break the functionality required to register component suits like Syncfusion. See "Register The Syncfusion License" at: https://blazorhelpwebsite.com/ViewBlogPost/47

@hishamco
Copy link
Contributor Author

How this break your functionality while registering Syncfusion done in the module project?

@ADefWebserver
Copy link
Member

How this break your functionality while registering Syncfusion done in the module project?

@hishamco - I am not certain it will break (I just hope not) :)

I was concerned that if Oqtane is not able to call this method signature, this method may not work anymore:

        public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
        {
            // Register Syncfusion license
            // Get a free license here:
            // https://www.syncfusion.com/products/communitylicense
            var builder = new ConfigurationBuilder()
                .SetBasePath(env.ContentRootPath)
                .AddJsonFile("appsettings.json", optional: false,
                reloadOnChange: true);
            var Configuration = builder.Build();
            var SyncfusionLicense = Configuration.GetSection("SyncfusionLicense");
            if (SyncfusionLicense != null)
            {
                Syncfusion.Licensing.SyncfusionLicenseProvider
                    .RegisterLicense(SyncfusionLicense.Value);
            }
        }

The code for the module is attached:

Syncfusion_Oqtane_Module.zip

@hishamco
Copy link
Contributor Author

No worry the Configure method will still called using another extension that loops through all custom startup classes

@ADefWebserver
Copy link
Member

No worry the Configure method will still called using another extension that loops through all custom startup classes

@hishamco - Thank you for looking at this :)

@hishamco
Copy link
Contributor Author

I need to resolve the conflict after merging the multi database support PR

@hishamco
Copy link
Contributor Author

hishamco commented Jun 4, 2021

@sbwalker do you have any feedback about this PR?

@sbwalker
Copy link
Member

@hishamco my apologies for taking so long to get to this... I have reviewed your PR and it does make startup a lot cleaner and should not have any compatibility issues as the ordering of operations has not changed. Please resolve the latest conflicts and I will merge.

@hishamco
Copy link
Contributor Author

I will try to resolve the conflict, this need a little time, because one mistake may break everything ;)

# Conflicts:
#	Oqtane.Client/Program.cs
#	Oqtane.Server/Startup.cs
@hishamco
Copy link
Contributor Author

Done from my side

var jsRuntime = serviceProvider.GetRequiredService<IJSRuntime>();
var interop = new Interop(jsRuntime);
var localizationCookie = await interop.GetCookie(CookieRequestCultureProvider.DefaultCookieName);
var culture = CookieRequestCultureProvider.ParseCookieValue(localizationCookie).UICultures[0].Value;
Copy link

Choose a reason for hiding this comment

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

today if I try to initialize Oqtane (first start) and use "Runtime": "WebAssembly" I'll receive something like
image

this is the fix
var culture = CookieRequestCultureProvider.ParseCookieValue(localizationCookie)?.UICultures?[0].Value;

would not like to add separate PR in order to avoid merge conflicts
@hishamco could you please modify your PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Openning a new issue is better for tracking

Copy link

Choose a reason for hiding this comment

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

agree, done

@sbwalker sbwalker merged commit 4a11524 into oqtane:dev Jun 15, 2021
@hishamco hishamco deleted the clean-startup branch June 15, 2021 11:46
@sbwalker
Copy link
Member

@hishamco some changes were lost in this PR - specifically see how the HttpClient is constructed here ( bc72055#diff-2fbf541e290883c7d8ad8f0bf02d667e2a4a89b5485771e05231ee1a91f2e377 ) - it contains newer code than your PR. I feel like I need to roll back this PR.

sbwalker added a commit to sbwalker/oqtane.framework that referenced this pull request Jun 15, 2021
sbwalker added a commit that referenced this pull request Jun 15, 2021
add logic removed in #1245 back to HttpClient creation
@sbwalker
Copy link
Member

@hishamco it looks like the only code mistakenly removed was in the HttpClient creation so I added it back

@sbwalker
Copy link
Member

There is still an issue in Visual Studio related to AddOqtaneScopedServices() where the tool is not able to resolve the reference - but the code still compiles fine?

image

sbwalker added a commit to sbwalker/oqtane.framework that referenced this pull request Jun 15, 2021
sbwalker added a commit that referenced this pull request Jun 15, 2021
added back missing ITenantManager registration removed in #1245
@sbwalker
Copy link
Member

the ITenantManager service was also removed from the PR which broke the application.

@hishamco
Copy link
Contributor Author

Let me double check before we rollback

@sbwalker
Copy link
Member

@hishamco I already dealt with the 2 issues I found - they are linked to this PR

@hishamco
Copy link
Contributor Author

Thanks, BTW there are some changes happen in translations and static resource I need to follow up why this happened especially static resources

@sbwalker
Copy link
Member

sbwalker commented Jun 15, 2021

@hishamco I will be adding the default resource files ( *.resx ) to the Oqtane.Framework project this week - someone is working through them right now before the PR is submitted to:

  • ensure all localized text are in the resx file
  • lookups use static keys rather than long descriptions
  • common terms use the SharedResources concept

Going forward all UI changes will need to have a corresponding update to the resx files.

@hishamco
Copy link
Contributor Author

Why we need to generate the neutral resx while the default localisation APIs shows the missing resources?

@sbwalker
Copy link
Member

@hishamco see oqtane/oqtane.translations#34 for an explanation of the changes being made

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

4 participants