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

Migrate to be Ember Octane app #99

Merged

Conversation

patocallaghan
Copy link
Collaborator

First off, apologies for the sheer size of this PR. I know this is potentially unreviewable but not sure of another way to do it 🙈

Some backstory. I wanted to implement an Octane version of the Ember Realworld application. I spoke to folks a while back and a separate application was suggested which was specifically based on the Octane edition, separate from the “Classic” Ember app. So I went off, and while I initially used the Ember Realworld fork as a base, I actually ended up rearchitecting and reimplementing a lot of what was in the Ember Classic application to follow a more component-driven architecture. This meant the codebase looked quite different than what was originally implemented. You can find that repo at patocallaghan/ember-octane-realworld.

I created an issue in Realworld for Ember Octane and submitted this app and it was accepted. You can currently see it in the framework list and the actual app is running at https://ember-octane-realworld.netlify.com/.

image

It has since been suggested on Discord by @GCheung55 and @trek that this should actually be part of the main Ember Realworld because it’s still the same framework and just an update in syntax.

Conversation from Discord below:
image
image
image

So what do people think? cc @Alonski @mansona and any others who have opinions. If we’re happy to merge this in I’d then submit a PR back to RealWorld to remove the entry to the Octane specific version.

@Alonski
Copy link
Member

Alonski commented Apr 27, 2020

Hmm this is very interesting. We could merge this in and then depreciate the Octane one.

@patocallaghan Maybe we can create an Octane branch here and split up this maybe PR into small ones? Then once that branch is ready we can merge into master?

'ember/order-in-routes': 'error',
'prefer-const': 'error',
'no-var': 'error',
'ember/use-ember-data-rfc-395-imports': 'error',
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why prettier/prettier, prefer-const and no-var were removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've simply used Ember CLI's 3.16 default eslintrc.js and just added the prettier and ember-data rules.

@@ -1,3 +1,3 @@
printWidth: 120
printWidth: 100
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for this?
Personally I have always used 120 that is why this was at 120.
Is this just a personal preference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah no reason other than I think this the default in Prettier and what I had in the Octane repo. Happy to change back.

@Alonski
Copy link
Member

Alonski commented May 22, 2020

@patocallaghan Can you try to get Netlify deploy preview working?
After that lets just merge this in!

@patocallaghan
Copy link
Collaborator Author

@Alonski good to go now! I just needed to sort the node version on netlify. Preview is here https://deploy-preview-99--ember-realworld.netlify.app/

Copy link
Member

@Alonski Alonski left a comment

Choose a reason for hiding this comment

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

Let's get this merged!

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