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

Upgraded view components sample projects to 2.2 instead of 2.0 and renewed seeding #10688

Merged
merged 2 commits into from
Jan 30, 2019
Merged

Conversation

shadialnamrouti
Copy link
Contributor

Fixes #10657

@guardrex
Copy link
Collaborator

The 2.2 updates should include the layout script updates. Was there an Error page (Error.cshtml.cs)? ... that needs a touch, too, if it's present in the sample. There are some appsettings file changes.

We decided to not sweat the cookie validation (CookiePolicyOptions and UseCookiePolicy) and the enforcement of HTTPS with HSTS, so that's fine if it wasn't there before.

@guardrex
Copy link
Collaborator

The Error page is static, so that's ok.

The scripts and styles in Shared/_Layout.cshtml ... you could update those, but I gravely doubt that it will matter all that much.

@guardrex
Copy link
Collaborator

@shadinamrouti, there's no migration required for OnModelCreating to seed the dB, correct?

@shadialnamrouti
Copy link
Contributor Author

@guardrex, I suggest to merge my PR because it is an identical working mirror of the current samples with the only differences being:

  1. Targeting the 2.2 libraries.
  2. Using the OnModelCreating for seeding.

Regarding your comments:
I. Additional files:

The original samples do not contain any of the following (it seems on purpose to concentrate on the View Component topic):

  • Views: The _layout, the _ViewImports, the Error View, navigation
  • Settings: appsettings.json, HTS, cookie policy
  • Static files: www root, site.css, site.js and the libraries

Notes:

  • None of the above files is mandatory to have a working sample.
  • Avoiding the above at this time is recommended to make the samples compatible with the documentation on the page.
  • Anyway, if you recommend adding the above, I can manage this in a subsequent PR. What do you think?

II. OnModelCreating() to seed in-memory DB's

Yes @guardrex , no migration is required because the used database is in-memory and thus migration is not possible in this context, however, to invoke OnModelCreating() an additional line is added in the constructor of the ToDo Controller which is:

public ToDoController(ToDoContext context)
{
            _ToDoContext = context;

            //This line is used to invoke OnModelCreating() for in-memory databases 
            _ToDoContext.Database.EnsureCreated();
}

For more info about EnsureCreated() see this related issue:
dotnet/efcore#11666

@guardrex
Copy link
Collaborator

guardrex commented Jan 30, 2019

Targeting the 2.2 libraries.

We don't retain the prior patch release sample; so yes, we'd drop 2.1 for 2.2. 👍

if you recommend adding the above

I would just update the scripts to match the 2.2 template to take advantage of any security/bug fixes. I gravely doubt that it's a big deal in a sample like this. I wouldn't do anything else for this one.

no migration is required

🎉 🍻 ❤️ 👍

There's a seeding technique that does require a migration, and I guess ur saying that for a data store not in-memory that this approach would require a migration? A migration isn't the most fun imo. I want to pull down a sample, fire it up, and have it ✨ Just Work!™️ ✨ ... no ceremony ... no distractions.

Although it's no biggie to do a migration, I just like the idea of not doing one ... LazyRex 😀

Approval

Given the seeding approach here, I defer to @Rick-Anderson and the team to sign off on this.

@Rick-Anderson Rick-Anderson merged commit 28e6882 into dotnet:master Jan 30, 2019
@Rick-Anderson
Copy link
Contributor

@shadinamrouti nice work.

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.

Upgrade view-components samples to 2.2 instead of 2.0
3 participants