Glimmer components migration

[cah-brian-gantzler]

It seems that there are some issues with glimmer, co-location, and the current structure that are incompatible. There was an old issue since fixed, ember-cli/ember-cli-htmlbars#265, but I think this repo has a nuance that wasnt fixed. Im not sure if its worth reporting or just altering this repo. (Spend a couple hours trying to figure out why things didnt work when converting to glimmer)

The nuance seems to be it is using pods syntax without actually saying it's using PODS (even though it works) and possibly that the components are nested in sub directories. (The super rentals demo shows glimmer working with pods, but it declares PODS and the components are not nested)

I experimented with converting the body component and it doesnt render unless we do one of the following. Rename template.hbs to component.hbs, or move both files up one directory and rename as body.js and body.hbs. I think the moving of the files and naming appropriately is the way to go. Is that ok with you?

[miguelcobain]

Nested "pod" glimmer components work if you name them index.js and index.hbs inside the folder with the component name.

But I think naming both files as component-name.js and component-name.hbs is more standard and I don't see why we shouldn't do it that way.
This approach still supports nesting in folders.

[cah-brian-gantzler]

Yes it did work nested when I renamed the template to component.hbs, which is basically the same as renaming them both to index.xxx they just have to have the same name where ever it is.

K, so for example, the body component will not be in the body dir anymore but up one dir and named body.xxx, but will still be nested in the yeti-table dir.

You can either close this issue or leave it open to track converting the comps to glimmer. I would like to convert them a couple at a time starting at the lower comps rather than do a massive PR.

[cah-brian-gantzler]

I cant find it listed anyway, what is the earliest version that yeti-table supports? Ember-try starts at 3.16, is that it? or should there be some additional trys added for the earlier versions?

[cah-brian-gantzler]

Not sure also how to allow documenting but like the following code is no longer valid under glimmer as the onRowClick will be no longer on the backing store but referred to as this.args.onRowClick. If I delete the below code will that effect the documentation at all?

  /**
   * Adds a click action to each row, called with the clicked row's data as an argument.
   * Can be used with both the blockless and block invocations.
   */
  onRowClick;

Also the linter says not to create empty backing classes (we can ignore of course) but that means addon docs doesnt have a place to get docs from code. Hmmm,

[miguelcobain]

@bgantzler I think it is possible to keep the jsdoc comment without an actual variable declared, but you need to add the @argument tag to it. Might as well add @type as well.

[cah-brian-gantzler]

What version of ember do you plan on supporting back to? Looking at updating some of the syntax and removing unneeded polyfills

[miguelcobain]

@bgantzler I would be ok with officially supporting whatever versions latest ember-try tests cover.
This will be a major bump, so we should be safe.

Additionally, we could try adding some scenarios for older versions and see how far can we go without breaking.

[cah-brian-gantzler]

Current dependencies are

"@ember/render-modifiers": "^1.0.2", 2.18+
"ember-decorators": "^6.1.1",
"ember-decorators-polyfill": "^1.1.5", 2.18+
"ember-fn-helper-polyfill": "^1.0.2", 2.12+
"ember-native-class-polyfill": "^1.0.6", 3.4+
"ember-on-modifier": "^1.0.1" 2.13+

So..... 3.4 is the farthest possible because of dependancies. that means we can get rid of most the get(this, "prop") syntac in favor of this.prop. I say most because any access to user supplied data will still need to use get as ember data uses proxies

Ill add some tries for 3.4 +

[cah-brian-gantzler]

Ok so they way the files were named originally did compile under 3.12, but the way they are named now (which Im pretty sure is the glimmer way of doing things) will NOT compile under 3.12. get the following

Build Error (TemplateCompiler)

EEXIST: file already exists, symlink '/var/folders/76/pygfd3k914100s1sf9x8xp2w0000gn/T/broccoli-13285fJXWNkXQgBG2/out-151-funnel_funnel_ember_yeti_table_addon/ember-yeti-table/components/yeti-table.js' -> '/var/folders/76/pygfd3k914100s1sf9x8xp2w0000gn/T/broccoli-13285fJXWNkXQgBG2/out-152-template_compiler/ember-yeti-table/components/yeti-table.js'

So since ember try was only running 3.16 and later didnt notice.

Since you said you were fine with officially supporting what ember-try scenarios were in place (3.16+), Im am going to go with that. @miguelcobain Please confirm or reject as soon as you can as I will be removing some of the polyfills that are no longer needed

[cah-brian-gantzler]

@sly7-7 Once the the above PR is merged, I will resubmit the PR for Body, TBody, Tbody/row and cell, I was then going to do TFoot/row/cell set and pagination. If you want to do those as a PR ill move onto the header set.

[sly7-7]

@cah-briangantzler I can at least try to make this PR concerning TFoot/row/cell. I think I must wait for #352 before beginning.
As a side note, it seems like all the row components share the same cell (un)registration. Do you think we could/should factorize this ? Maybe it's not a good idea, neither in the scope of the migration.

[cah-brian-gantzler]

If you mean the code is the same, actually some of it should not be, the if for prop https://github.com/miguelcobain/ember-yeti-table/blob/master/addon/components/yeti-table/tbody/row.js#L60 should ONLY be in the Thead column registration, prop is never passed on body or foot cell, prop is derived from column which is what the else is looking up, so Im removing that in body/row and it should also be removed in foot/row. That does leave them looking the same, but I would leave them as separate functions to allow them each to do special processing should the need arise. There isnt now, but could be later.

I dont think you have to wait for #352. Up to you. I already have the body/row/cell coded, after that PR is merged I will rebase my branch. What ever you feel comfortable with. In the one PR you had offered to help, so trying to make sure you have the opportunity.

[sly7-7]

Thanks for the feedback. So I'm doing tfoot/row/cell right now.

[cah-brian-gantzler]

So down to three components left, yeti-table, column, pagination. These will take a little longer due to the complexity. Wont see some PRs quite as quickly as the other ones :)

[miguelcobain]

@cah-briangantzler what's your strategy for observing changes to the given @data array?

Will auto-tracking get us covered without doing much? Maybe that's the case and user's are the ones responsible for setting up @tracked where they want to see changes reflected.

Almost all of the complexity on the main component comes from creating dynamic computed properties. Not sure how this will port to glimmer components yet.

So, because auto-tracking is based on access and not on declaring the dependencies, it might just work™ by defining normal getters.
Of course, if the user wants to see changes reflected, the given array and/or it's content properties should be @tracked.

[cah-brian-gantzler]

If they create objects as @Tracked were covered, Ember Data is auto tracked, I think the question here is how to support an array of POJOs and the answer is we still create the computed properties or we use something like https://github.com/pzuraq/tracked-built-ins which does not support IE 11 (Proxies). Would that be a problem?

If we dont create the computeds and depend on the user doing @Tracked, thats a pretty big breaking change. You can still convert an object to glimmer and use computeds, @Tracked is just a sub feature provided by glimmer.

the glimmer complexity I have to work through is more the one-way bound, this.args, and providing a default value for optional passed params.

[miguelcobain]

I think the new "mental model" that octane is pushing forward is to start thinking about annotating the data source and forgetting.
So, in a sense, this new mental model problem pushed this concern outside of yeti-tables into user land (where the source is).

tracked-built-ins is something that the user could consider using, but there are other approaches like using EmberArray and EmberObject which are also compatible with auto tracking and are IE11 compatible (or perhaps even using normal POJOs if they don't care about updating data?).

I do agree that it would be a breaking change, but I'm more concerned about what is the correct approach and no so much about breaking changes since we're major bumping.

If you make a table component yourself in your own octane codebase and pass it some array of POJOs, things won't automatically update either.

This topic is still a bit unclear to me.

[cah-brian-gantzler]

Agree with all that. The breaking change would be either user the tracked-built-in or change your POJOs to objects using tracked. Just thinking that may be a big barrier to people upgrading.

I would prefer to create the computeds, in this release so people could upgrade fairly seamlessly, then deprecate the creation of the computeds and remove them later, giving people a longer runway to prepare.

Im not sure I would be able to upgrade yeti-table for a while if we did away with the computeds. I cant use the tracked-built-in yet (IE11 sigh), so I would have to change all my POJOs to class objects to upgrade (changing them to emberObject would be close to the same about of work. I use yeti-table a lot (as evidenced by the PRs here). I dont know how many of my tables just display data vs needing to be reactive.

Specifically why I am saving that to the last thing to convert and getting the easy stuff out of the way.

[miguelcobain]

If you don't know if you can upgrade yet, you could always stay on a previous version (we can still backport bugfixes and maybe even some small features).

You would still benefit from the components just being glimmer components, since they're quite faster. So, I can see your point of a semi-octane release making sense.

What about some hybrid way? Perhaps only creating those computed properties based on some flag?
This would probably add a lot of complexity. We could almost have two separate main components that yield the same child components.

Another option would be to release 2.0 ("semi octane") and the 3.0 (octane) right after?

People like you could stay on 2.0.

Sorry for the random written thoughts, but I'm trying to make sure we're not missing anything.
I think new people coming to ember now should be encouraged to build their codebases around octane idioms, which is the official way now.

[cah-brian-gantzler]

I had already planned on a flag that created the computeds, if the flag was off, no computeds. That way we could actually issue a deprecation. The deprecation would be the flag is on, creating the computeds and this would be removed in the next major release. This is all theory though as I havent converted it yet and who knows, may come up with a clever idea when we get to that point.

[cah-brian-gantzler]

Waiting on approval of #376

Working on yeti proper and finding that in your test you do @registerApi={{fn (mut this.tableApi)}}. This is giving me a problem now in glimmer because this syntax uses this.tableApi then modifies it. This throws an warning about updating a property after it has been used.

I can update the test, but just wondering if anyone is actually do this. They will need to change to either an actual method that updates the value or @registerApi={{action (mut this.tableApi)}}

This is an actual change required by glimmer, not something we are actually doing in this repo.

[miguelcobain]

I am using @registerApi={{action (mut this.tableApi)}} in a production codebase.
It's not entirely clear to me why @registerApi={{fn (mut this.tableApi)}} doesn't work but @registerApi={{action (mut this.tableApi)}}. 🤔

This is an actual change required by glimmer, not something we are actually doing in this repo.

I use this {{fn (mut combo a lot in various places, so I wasn't expecting problems with that. 😕

[cah-brian-gantzler]

It appears that the fn version reads the value in addition to changing it, where the action version does not. Not sure why. But reading the value first is what throws the warning that you accessed a value during the same render as changing it. Just wanted to make sure it was noted somewhere

[miguelcobain]

@cah-briangantzler as you said, this is something that isn't directly related with ember-yeti-table, so we should just fix it in some way here.

[cah-brian-gantzler]

What is the intent of the test loadData is called once if we change @filter from undefined to "". I would think that the loadData would be called once on the initial render and then once on the re-render since @filter was changed. Is there some code that equating undefined with "" and then suppressing the second call? In glimmer remember that @Tracked does not check if the value is different but that it was assigned. (Yes the tracked toolbox has @dedupeTracked) but in this case it really did change.

Is this a test I need to honor or can this test be removed?

[cah-brian-gantzler]

@cah-briangantzler as you said, this is something that isn't directly related with ember-yeti-table, so we should just fix it in some way here.

This is used only in our tests, and I did fix the tests. I just wanted to mention it so if anyone in their code was using the fn version that had an issue that would trigger on a search to help then understand why they were having a probem

[miguelcobain]

@cah-briangantzler The purpose of that test is to improve interoperability with inputs. Basically, to facilitate situations like the following:

<Input @value={{this.filterText}}/>

<YetiTable @loadData={{this.loadData}} @filter={{this.filterText}} as |table|>
  {{!-- ... --}}

In this case, {{this.filterText}} starts as undefined. However, if you click the input and click outside without typing anything, the input's @value will change to '', which triggers a @loadData run unnecessarily.

There is some code to equate undefined with ''. This is the relevant commit that added the test: e7502d3

Basically, we default the filter value to '' when it is undefined. That way, the input can change to '' and it won't trigger a change. Of course, with @tracked properties, the story might be different since, as you said, setting to the same value does trigger a change. In this case, the @dedupeTracked might be the best fit.

Let me know if this answered your question.

[cah-brian-gantzler]

Ok so setting the default of '' should handle most of it, but setting @dedupeTracked with handle the rest.

filter was the only prop that had this test, but I also assume that means things like sort, pageNumber, etc should also use @dedupeTracked as well.

[miguelcobain]

Well, I guess it wouldn't "hurt", but @filter was a more glaring issue since it is so usual to connect it to an input.

[cah-brian-gantzler]

Interesting. You can not combine @localcopy with @dedupeTracked

You attempted to use @tracked on filter, but that element is not a class field. @tracked is only usable on class fields. Native getters and setters will autotrack add any tracked fields they encounter, so there is no need mark getters and setters with @tracked.

Either we let loadData be called twice or I have to use my version of the code instead of @localcopy.

Basically you cant dedupe an this.args property, which kinda makes sense.

[cah-brian-gantzler]

I would also purpose to do away with many of the defaults in code and just add them to the default theme. For example filter, if it was added to the default config as "", you wouldnt need to worry about the defaulting in code as it would be handled the same way as all the other user supplied defaults

[miguelcobain]

@cah-briangantzler I guess we need to find a way add some "gate" somewhere and only allows changes when the values are different.

[miguelcobain]

Right now the filter property is used on an ember computed. Are you migrating that to a native getter?
I guess the easiest way would be if you could put up a PR with your preferred solution and that would serve a bases for the discussion.
Hopefully it wouldn't be too much work to "change gears" later if we decide we need to.

[cah-brian-gantzler]

Interesting. You can not combine @localcopy with @dedupeTracked

Scratch that. There is a variation of @localcopy I should be using. Will let you know how it goes

[cah-brian-gantzler]

Working on yeti proper and finding that in your test you do @registerApi={{fn (mut this.tableApi)}}. This is giving me a problem now in glimmer because this syntax uses this.tableApi then modifies it. This throws an warning about updating a property after it has been used.

Its a little bit of reading, but I think the answer is in here. {{mut}} is doing far more than you thought it was. https://www.pzuraq.com/on-mut-and-2-way-binding/

[miguelcobain]

v2.0.0 was released with all components as glimmer components! 🎉
Thanks @cah-brian-gantzler !