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

Ember Table 4.0 - with support for Ember 4.0 #919

Merged
merged 26 commits into from
May 18, 2022
Merged

Ember Table 4.0 - with support for Ember 4.0 #919

merged 26 commits into from
May 18, 2022

Conversation

kpfefferle
Copy link
Member

@kpfefferle kpfefferle commented Dec 3, 2021

The goal of this branch/PR is:

  • Drop Ember 2.8 & 2.12 support. The minimum supported version of Ember for ember-table 4.0 will be 2.18.
  • ember-cli-page-object will block supporting Ember 4 in the test helpers published with ember-table. There are a few options: Fix it upstream (there are some efforts), drop the dependency, drop the test helpers (this last option is unlikely to be the best as Addepar uses these extensively).
  • Re-enable the ember-release scenario which exercises Ember 4.0

In order to ship a release that supports Ember 4.0, we first will need to address any 3.x deprecations. #897 is a list to start with. PRs targeting this branch that move us in this direction are welcome.

Breaking change PRs for documenting:

The following upstream dependencies should be upgraded to stable releases when they occur:

  • ember-classy-page-object 0.8
    • depends on ember-cli-page-object 2.0
  • @html-next/vertical-collection 3.0

Resolves #897

@kpfefferle
Copy link
Member Author

I just published 4.0.0-beta.0 as an initial prerelease

@MichalBryxi
Copy link

While trying the beta version:

    "ember-table": "4.0.0-beta.1",

I bumped into following error while running ember server:

❯ yarn start
yarn run v1.22.11
$ ember serve --proxy=http://localhost:3000

this.registerPlugin is not a function
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I think I maybe might have fixed this by putting following to package.json:

  "resolutions": {
    "ember-cli-htmlbars": "6.0.1"
  },

@MichalBryxi
Copy link

Another hurdle:

❯ yarn start
yarn run v1.22.11
$ ember serve --proxy=http://localhost:3000

Template Compiler Error (TemplateCompiler) in ember-table/components/-private/scroll-indicators/template.hbs

Cannot set property 'syntax' of undefined

Got "fixed" by putting following in package.json:

  "resolutions": {
    "ember-test-selectors": "6.0.0"
  },

@kpfefferle
Copy link
Member Author

@MichalBryxi Are you trying this with Ember 4? The errors you're seeing appear to be things that have not yet been addressed in this PR yet to work with Ember 4. The goal of this PR is to eventually get us there, but at this time even this beta branch is not yet compatible with Ember 4.

@MichalBryxi
Copy link

@kpfefferle correct. I thought maybe this information might be helpful for someone trying to make it Ember-v4 compat.

package.json Show resolved Hide resolved
@mixonic
Copy link
Member

mixonic commented Jan 15, 2022

I've made fairly extensive updates to this branch this evening. The test suite and dependencies should be pretty much ready for Ember 4.0, although I'm not saying Ember 4.0 is green, or even that tests are totally unblocked.

  • There probably remain tests using deprecated behaviors. These should be debugged and resolved on the 3.28 builds before wrestling with Ember 4.0 IMO.
  • ember-cli-page-object will block supporting Ember 4 in the test helpers published with ember-table. There are a few options: Fix it upstream (there are some efforts on their repo), drop the dependency, drop the test helpers (this last option is unlikely to be the best as Addepar uses these extensively).
  • There may be a complication around ember-test-selectors as well, I'm unsure.

Leaving it there for now. It should be a good foundation for someone else to dive into at this point.

@kpfefferle
Copy link
Member Author

kpfefferle commented Feb 16, 2022

@DanMonroe @mixonic Do any of the recently merged PRs need to be added to this PR's description list for specific documentation as breaking changes?

DanMonroe and others added 2 commits February 24, 2022 10:09
* Add ember-try scenario for ember v4

* Add ember-4.0 to ci.yml

* resolving v4 dependancies

* Reverse dependency resolutions for ember-classy-page-object in ember-try

* Use ember-classy-page-object v0.8.0-beta

* Fixed ember-try scenarios 3.24 and below

* Moved explanation of ember-classy-page-object resolution with <= 3.24

* Re-enabled ember-try release scenario
@kpfefferle
Copy link
Member Author

kpfefferle commented Feb 24, 2022

4.0.0-beta.4 has been released with full Ember 4 compatibility. We'll likely keep this in beta for a bit so people can try it out. Ideally, I'd hope to see the following dependencies release stable versions before we promote ember-table v4 to stable:

  • ember-classy-page-object 0.8
    • depends on ember-cli-page-object 2.0 (edit: @mixonic suggests we can pin classy page object to the beta, no need to block on a final major version bump for this dependency)
  • @html-next/vertical-collection 3.0

@vlascik
Copy link

vlascik commented Apr 28, 2022

How about forwarding ...attributes to table, th, tr, tbody etc? Would make styling and integrating with frameworks like Bootstrap and Tailwind a lot easier.

@mixonic
Copy link
Member

mixonic commented May 2, 2022

Apologies to the vested parties for the hold-up. In our test suite at Addepar we noted some layout changes which were unexpected. It appeared that some columns were not keeping the same widths as in Ember Table 3, and row height might also be very, very slightly adjusted.

I was able to track this down today. In vertical-collection 2.0.0 (which our app relied upon) there was an errant , in app.css. This was resolved before 2.0.1: html-next/vertical-collection@fbb029c

That extra comma caused a syntax issue in CSS, and effectively disabled those styling rules. This led the display of the occluded block to be block, and the max-height directive to have an impact (which it does not after the display; table-row from the disabled style block). That in turn impacts the sticky polyfill's calculation of "scale" for layout at https://github.com/Addepar/ember-table/blob/master/addon/-private/sticky/table-sticky-polyfill.js#L174, corrupting columns and other sizes in very subtle and floating-point level ways.

Adopting the upgrade of ember-table bumped the vertical-collection version in our app, thus correcting the styles and re-enabling the display: table-row styles, and we got visual regression failures.

Having nailed that down, I believe we've addressed all concerns here. If there is any further feedback on getting 4.0 please chime in ASAP, if there is nothing new we will pull the trigger this week (hopefully tomorrow).

Thank you!

@mixonic mixonic merged commit e90a367 into master May 18, 2022
@mixonic mixonic deleted the 4.0-beta branch May 18, 2022 13:34
@mixonic
Copy link
Member

mixonic commented May 19, 2022

Ember 4.0 stable has been released! Thank you to to many contributors and stakeholders involved in this 🙇

I would like to move forward pretty quickly with Ember Table 5.0. This would drop Ember 2.18, making the minimum supported Ember version 3.4-LTS. It would raise the minimum Node version. It would perhaps disable the "detect scaling styles" logic by default (as it corrupts layout logic) and make handling scaled UI opt-in (where it is currently a default behavior today). I'll open an issue soon, and I'm working on some of the scaling-related code changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support Ember 4.0
6 participants