imgix / ember-cli-imgix

Easily add imgix functionality to your Ember application
https://imgix.github.io/ember-cli-imgix
MIT License
26 stars 13 forks source link

feat: update-ember-cli-project #213

Closed rahulk94 closed 2 years ago

rahulk94 commented 2 years ago

Description

This PR bumps this project up to the latest Ember CLI blueprint. This change has primarily been done using ember-cli-update followed up by commits to update tests and the source to follow newer lint rules.

This should be pretty straightforward / match other Ember CLI updates for addons as I've tried to do keep things as standard as possible here.

Not all lint rules have been addressed at this stage, I think upgrading the CLI version and later addressing lint rules is probably a more tactiful approach.

The tests are passing with Ember@3 LTS versions and failing with Ember@4 versions (which is fine since we're not aiming to add support for that in this PR as it drops lots of deprecated functionality). Embroider safe and optimized builds also fail atm and I'll do a separate PR to address that (as I think there's more issues around Babel with Embroider too).

New Feature

Steps to Test

Running the tests and also the dummy app in this project should work as expected.

I've also integration tested this with our internal Ember app and verified this works as expected. Unfortunately I can't share that as it's closed source.

commit-lint[bot] commented 2 years ago

Contributors

rahulk94

Commit-Lint commands
You can trigger Commit-Lint actions by commenting on this PR: - `@Commit-Lint merge patch` will merge dependabot PR on "patch" versions (X.X.Y - Y change) - `@Commit-Lint merge minor` will merge dependabot PR on "minor" versions (X.Y.Y - Y change) - `@Commit-Lint merge major` will merge dependabot PR on "major" versions (Y.Y.Y - Y change) - `@Commit-Lint merge disable` will desactivate merge dependabot PR - `@Commit-Lint review` will approve dependabot PR - `@Commit-Lint stop review` will stop approve dependabot PR
frederickfogerty commented 2 years ago

To be clear here, I'm pretty happy with the state of this PR. If we can get to a resolution about the Travis build and what to do to make sure it passes, then I'm happy to merge this.

rahulk94 commented 2 years ago

Hey @frederickfogerty thanks the quick first pass and getting the build to run on Travis.

Take a look here for failing tests


=== Scenario: ember-lts-3.20 ===================================================
Result: true

=== Scenario: ember-lts-3.24 =================================================== Result: true

=== Scenario: ember-lts-3.28 =================================================== Result: true

=== Scenario: ember-release ==================================================== To use these addons, your app needs ember-auto-import >= 2: ember-source Result: false

=== Scenario: ember-beta ======================================================= To use these addons, your app needs ember-auto-import >= 2: ember-source Result: false

=== Scenario: ember-canary ===================================================== To use these addons, your app needs ember-auto-import >= 2: ember-source

Result: false

=== Scenario: ember-classic ==================================================== Result: true

=== Scenario: embroider-safe =================================================== No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself. Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received The build has been terminated



Yeah so I'm not 100% sure what the best approach here is atm the moment. The Ember LTS series against version 3 all pass which is great, but the release, beta and canary are all failing due to us still using [ember-auto-import (EAI)](https://github.com/ef4/ember-auto-import) 1.x. EAI has recently had a major release which is not backwards compatible (I'll let you skim the [upgrade/migration summary section](https://github.com/ef4/ember-auto-import/blob/main/docs/upgrade-guide-2.0.md) rather than try re-explain it) and so that fails those builds. Ember release/beta/canary are all running Ember@4 as well which drops deprecated functionality (I have not checked if we are doing anything thats been dropped just yet). Embroider Ember Try builds are expected to fail at this stage (I haven't tried getting compatibility going and they just timeout weirdly).

Summing up, what are your thoughts on commenting the release, beta, canary, and Embroider options out in the Ember Try config and cutting a 3.0.0-beta.1 style release? I can then look at making Embroider options work since thats what I'm hear to try solve πŸ˜‚  and do a 3.0.0-beta.2 and then we can see if we want/need to do anything else before cutting a 3.0.0 release?

Ember@4 support can be discussed in a later PR but depending on how easy it is/my capacity I may cheap out and just do a README note and Issue saying Ember we need to add Ember@4 compat soon.
frederickfogerty commented 2 years ago

Right, that makes sense as to why those builds are failing then, and I'm happy to comment out those stages and proceed as you suggested. It seems that the ember try environment variable (EMBER_TRY_SCENARIO) isn't working properly, I will need to fix this before merging as well. Do you have any ideas how to fix this? I could also just use one ember try since it seems that is already doing all of the steps πŸ˜…

Totally understand about Ember 4, and thanks for all your help until now!

rahulk94 commented 2 years ago

Hey @frederickfogerty I've removed the Ember@4 and Embroider stuff per our earlier discussion.

I've also updated the Travis.yml to call yarn test:ember-compatibility in the second stage so it should call Ember Try now, whereas earlier I think it was just doing ember test again πŸ˜… . I reckon start the build and lets see how far we get now.

Oh also, commit lint is complaining at me but I can't tell what I've done wrong since it just links to generic docs rather than the broken rule so if you can tell let me know and I can touch it up πŸ˜‚

sherwinski commented 2 years ago

Oh also, commit lint is complaining at me but I can't tell what I've done wrong since it just links to generic docs rather than the broken rule so if you can tell let me know and I can touch it up πŸ˜‚

@rahulk94 Maybe I can chime in here. Commit lint expects the PR title to conform to a specific format, detailed here. So if you were to edit the PR title that check should pass.

rahulk94 commented 2 years ago

@sherwinski πŸ€¦πŸΎβ€β™‚οΈ cheers mate... Had a / instead of a : πŸ₯²

frederickfogerty commented 2 years ago

Hey @rahulk94 - just leaving an interim comment here. I worked on Friday to try get the Travis build to pass, but didn't succeed. I will have a bit more time tomorrow, so hopefully I can have an update to you by then πŸ‘ You can track my progress here for the branch "upgrade/update-ember-cli"

frederickfogerty commented 2 years ago

I got the tests passing here so I'll go ahead and merge this πŸŽ‰

frederickfogerty commented 2 years ago

@rahulk94 I opened up this PR to release v3 of this library. If you have any improvements on the release notes there, I'm all ears. Thanks!