embroider support

[cah-brian-gantzler]

Embroider choose not to support the use of private components in the form of ember-yeti-table@component-name.

Unless there is a way to tell embroider how to recognize these components, may have to create a PR that makes all these components public again :(

Thoughts?

[cah-brian-gantzler]

Is this still being maintained? Master branch does not build on my local anymore, lotta dependancies out of date.

Still updating dependancies and trying to get this to build.

If this is not going to be maintained anymore, may just start with a new repo and add the components back til I get it work ing.

[cah-brian-gantzler]

Its possible Miguel may be on tour, hope to hear from him soon.

I have completely rewritten this addon using the new template imports, making it embroider compatible, and updating to glimmer components with tracked properties. This will cause some breaking changes as non-glimmer components were depending on two way binding and computed properties instead of tracked.

More updates to come

[miguelcobain]

@cah-brian-gantzler my apologies for being MIA. I'm totally interested in moving this addon forward to embroider as I'll be needing it myself at some point. I have a glimmer migration in progress for quite some time, but I'd love to see your rewrite, either in a PR or in a repo.

I was struggling with the feature to "stop watching updates". That's why I stopped working on it.

[cah-brian-gantzler]

I really love yeti-table over the other table addons and really wanted to keep it, so.....

I have created a completely new addon. I had thought that if you got back with me making a PR to this repo would be what I would do, but because some of the breaking changes will most likely effect every use of yeti, you would have to convert every use at once to migrate. But if its in a separate addon, you can actually include both and convert each use one at a time. Now that we have template imports, it seems a great time to do it.

So I think keeping it as a separate addon is the way to go. I just called it ember-yeti-table2 :) https://github.com/bgantzler/ember-yeti-table2 if you want to look. The read me contains some of the breaking changes I had to implement, mostly caused by glimmer and tracked.

I just got all the tests passing today. Working a little on the CI, but have not published it to npm at all.

I am actually going to try to convert a use or two in my own application (while keeping the previous version at the same time), so see how migration actually goes and can better write a guide and smooth any rough edges.

[cah-brian-gantzler]

I was struggling with the feature to "stop watching updates". That's why I stopped working on it.

If this means in relation to ignoreDataChanges, yea, thats a breaking change. Just can't really do it if they have tracked on a property and you really need to have tracked for the rest of your app. The effect can be done, but has to be on the part of the consumer (basically passing yeti a POJO of the entity that doesnt have tracked on their properties and then tying back the edit. Could be a lot of work, dont know yet ). I use that a few times in my app (really think this should be the default for all tables) so will see what it takes to implement.

[miguelcobain]

@cah-brian-gantzler this is my attempt at the migration: https://github.com/miguelcobain/ember-yeti-table/tree/octane

The only failing tests are:

• the ember-concurrency cancellation test - it is working correctly but, for some weird reason, the cancellation throws an error that the test catches and fails. This "error" is expected, but I don't know how to silence/ignore it.
• the two @ignoreDataChanges tests (I tried implementing this using untrack, but without success).

So, the only breaking change would be @ignoreDataChanges (and yes, data must be tracked). Would love to get your feedback.

[cah-brian-gantzler]

I didnt fully embrace trackedToolbox and ended up writing an equiv to localCopy annotation, but in code only. localCopy is better though in reflecting changes. Tracked firing even if changed to same value was the biggest headache and this fixes that as well

I also didnt use trackedFunction, was going to do that next, so glad to see you got it working.

In the tests I removed the A() an just used native arrays to help get off some embery things.

If all the tests are working, I would just go with your version for first step, can look at the cancellation test if you want. If ignoreDataChanges is the only breaking change I can live with that.

Referring to components like ember-yeti-table@yeti-table::thead isnt embroider compatible. (I know I suggested it, and rjwblue said it would be supported, but that was before embroider and we guessed wrong). We can make another commit after that is merged to fix all that. I embraced the template imports, which helped a lot, also did it in tests but had to change tests some as this is not the context anymore. Required me to create some temp objects to add tracked, but that more closely mirrored actual use so that was ok.

So if tests are working except for ignoreDataChange, I would say I would go with that. I have a couple ideas on how to bring that back, will be a lot of code and not sure it will work, but could iterate on that. Its really centered on fitleredData and sortedData and getting them to return previous results instead of computing new.

[cah-brian-gantzler]

while attempting to run pnpm i in the octane branch I get the following. So cant run tests. Any ideas?

node_modules/.pnpm/sharp@0.28.3/node_modules/sharp: Running install script, failed in 13.2s
.../sharp@0.28.3/node_modules/sharp install$ (node install/libvips && node install/dll-copy && prebuild-install) || (node install/can-compile && node-gyp rebuild && node install/dll-copy)
│ sharp: Using existing vendored libvips v8.10.6
│ prebuild-install WARN install invalid distance too far back
│ gyp info it worked if it ends with ok
│ gyp info using node-gyp@9.4.0
│ gyp info using node@20.3.0 | darwin | x64
│ gyp info find Python using Python version 3.11.4 found at "/Library/Frameworks/Python.framework/Versions/3.11/bin/python3"
│ gyp info spawn /Library/Frameworks/Python.framework/Versions/3.11/bin/python3
│ gyp info spawn args [
│ gyp info spawn args   '/Users/brian.gantzler/Library/pnpm/global/5/.pnpm/pnpm@8.6.10/node_modules/pnpm/dist/node_modules/node-gyp/gyp/gyp_main.py',
│ gyp info spawn args   'binding.gyp',
│ gyp info spawn args   '-f',
│ gyp info spawn args   'make',
│ gyp info spawn args   '-I',
│ gyp info spawn args   '/Users/brian.gantzler/source-personal/ember-yeti-table/node_modules/.pnpm/sharp@0.28.3/node_modules/sharp/build/config.gypi',
│ gyp info spawn args   '-I',
│ gyp info spawn args   '/Users/brian.gantzler/Library/pnpm/global/5/.pnpm/pnpm@8.6.10/node_modules/pnpm/dist/node_modules/node-gyp/addon.gypi',
│ gyp info spawn args   '-I',
│ gyp info spawn args   '/Users/brian.gantzler/Library/Caches/node-gyp/20.3.0/include/node/common.gypi',
│ gyp info spawn args   '-Dlibrary=shared_library',
│ gyp info spawn args   '-Dvisibility=default',
│ gyp info spawn args   '-Dnode_root_dir=/Users/brian.gantzler/Library/Caches/node-gyp/20.3.0',
│ gyp info spawn args   '-Dnode_gyp_dir=/Users/brian.gantzler/Library/pnpm/global/5/.pnpm/pnpm@8.6.10/node_modules/pnpm/dist/node_modules/node-gyp',
│ gyp info spawn args   '-Dnode_lib_file=/Users/brian.gantzler/Library/Caches/node-gyp/20.3.0/<(target_arch)/node.lib',
│ gyp info spawn args   '-Dmodule_root_dir=/Users/brian.gantzler/source-personal/ember-yeti-table/node_modules/.pnpm/sharp@0.28.3/node_modules/sharp',
│ gyp info spawn args   '-Dnode_engine=v8',
│ gyp info spawn args   '--depth=.',
│ gyp info spawn args   '--no-parallel',
│ gyp info spawn args   '--generator-output',
│ gyp info spawn args   'build',
│ gyp info spawn args   '-Goutput_dir=.'
│ gyp info spawn args ]
│ gyp info spawn make
│ gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build' ]
│   CC(target) Release/obj.target/nothing/../../../node-addon-api@3.2.1/node_modules/node-addon-api/nothing.o
│   LIBTOOL-STATIC Release/nothing.a
│ warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: archive library: Release/nothing.a the table of contents is empty (no object file members in the library define global symbols)
│   TOUCH Release/obj.target/libvips-cpp.stamp
│   CXX(target) Release/obj.target/sharp/src/common.o
│ ../src/common.cc:24:10: fatal error: 'vips/vips8' file not found
│ #include <vips/vips8>
│          ^~~~~~~~~~~~
│ 1 error generated.
│ make: *** [Release/obj.target/sharp/src/common.o] Error 1
│ gyp ERR! build error 
│ gyp ERR! stack Error: `make` failed with exit code: 2
│ gyp ERR! stack     at ChildProcess.onExit (/Users/brian.gantzler/Library/pnpm/global/5/.pnpm/pnpm@8.6.10/node_modules/pnpm/dist/node_modules/node-gyp/lib/build.js:203:23)
│ gyp ERR! stack     at ChildProcess.emit (node:events:511:28)
│ gyp ERR! stack     at ChildProcess._handle.onexit (node:internal/child_process:293:12)
│ gyp ERR! System Darwin 22.6.0
│ gyp ERR! command "/usr/local/Cellar/node/20.3.0_1/bin/node" "/Users/brian.gantzler/Library/pnpm/global/5/.pnpm/pnpm@8.6.10/node_modules/pnpm/dist/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
│ gyp ERR! cwd /Users/brian.gantzler/source-personal/ember-yeti-table/node_modules/.pnpm/sharp@0.28.3/node_modules/sharp
│ gyp ERR! node -v v20.3.0
│ gyp ERR! node-gyp -v v9.4.0
│ gyp ERR! not ok 
└─ Failed in 13.2s at /Users/brian.gantzler/source-personal/ember-yeti-table/node_modules/.pnpm/sharp@0.28.3/node_modules/sharp
 ELIFECYCLE  Command failed with exit code 1.

[miguelcobain]

Seems like some problem with a deep transitive dependency. https://github.com/lovell/sharp/issues/1882

It works fine here.

It's very likely a dependency of ember-cli-favicon.

[cah-brian-gantzler]

its says Im using the latest 3.0.0 of ember-cli-favicon. From the link, tried pinning mini-pass, no luck. As is, I can not install locally, so cant run tests, would hamper anything I would do.

In your package.json you have peer-dependency of ember-source 4, should widen and add 5 or do >= 4

Do you still need ember-decorators? @tagName can be removed with glimmer etc.

Would like to see auto import in the 2.0 series for better embroider support

[miguelcobain]

You could temporarily remove ember-cli-favicon to get past this?

Did you try rm -rF node_modules && pnpm install? I'm always amazed how much this still solves things in 2023.

ember-concurrency-decorators is being used in the docs. e-c 3+ has decorator support, but we can't use it because ember-cli-addon-docs is using 2.x, I think. But there's no hard dependency on it.

peer-dependency change you mentioned sounds fine. I just used whatever was in the 4.12 blueprint.

[bgantzler]

When I get home going to try to remove ember fav icon and see.

Yes did try removing node modules. Didn’t work.

Not ember-concurrency-decorators. There is ember-decorators. It’s where @tagName(‘’) comes from you have on yeti-table. With glimmer there is no wrapper div to get rid of

[bgantzler]

Was just checking addon docs and it can now be a standalone app instead of in dummy. I would look into changing that so this can be made a V 2 addon eventually

[cah-brian-gantzler]

Removing ember-cli-favicon does indeed allow me to run tests. Weird.

The task cancellation test, ran and debugged many times. The error I believe is actually being thrown here https://github.com/miguelcobain/ember-yeti-table/blob/octane/addon/components/yeti-table.js#L411 cause thats where the perform is being called again. However moving this inside the try doesnt seem to make a difference, the error is caught, not re-thrown, but message still appears in log.

I am assuming the test worked on the master branch, dont really see that anything is changed in how the promise is called that would cause it not to work now.

[cah-brian-gantzler]

I thought I had moved it into the try didnt work, but tried again and does. Use this code and the test passes for me. Left the comments in so you see the data.then, but dont really need it and makes it simpler, remove the commented out code of course

    // resolve the data promise (either from the @loadData call or @data)
    // if (data?.then) {
      try {
        if (typeof this.args.loadData === 'function') {
          let params = this.computeLoadDataParams();
          data = this.args.loadData(params);
        }
        data = await data;
      } catch (e) {
        if (!didCancel(e)) {
          // re-throw the non-cancellation error
          throw e;
        }
      }
    // }

[cah-brian-gantzler]

Ok, so changing the code to this actually works, not sure why, as I said moving the code into the try should have worked rather than creating a new try. But that is loadData being an ember-concurrency task. Since you allow @data to be a promise, a task could be passed in there, is there a test when @data is a task?

    if (typeof this.args.loadData === 'function') {
      let params = this.computeLoadDataParams();
      try {
        data = await this.args.loadData(params);
      } catch (e) {
        if (!didCancel(e)) {
          // re-throw the non-cancellation error
          throw e;
        }
      }
    }

    // resolve the data promise (either from the @loadData call or @data)
    // if (data?.then) {
      try {
        data = await data;
      } catch (e) {
        if (!didCancel(e)) {
          // re-throw the non-cancellation error
          throw e;
        }
      }
    // }

you'll also notice that testing for data.then doesnt really need to be done and just adds complexity for little gained.

[bgantzler]

Weird. I posted a more concise version of this but it doesn’t see. To be here. Out at tge moment. Will have to check again when I get home

[cah-brian-gantzler]

More concise

    // resolve the data promise (either from the @loadData call or @data)
    // if (data?.then) {
      try {
        if (typeof this.args.loadData === 'function') {
          let params = this.computeLoadDataParams();
          data = this.args.loadData(params);
        }
        data = await data;
      } catch (e) {
        if (!didCancel(e)) {
          // re-throw the non-cancellation error
          throw e;
        }
      }
    // }

[cah-brian-gantzler]

I fixed the ignore data changes. You can look at the code here https://github.com/bgantzler/ember-yeti-table/tree/ignore-data-changes

The key was to make processed data a tracked variable instead of a getter. Then using the functions as helpers to trigger the code, just dont return anything in the helper, but set processedData.

Then, alter the filter, sort functions. The were seperated, but I had to move all the code that access fields you want to change (column sorts, the filter, etc) from the code that actually does the sorting and the filtering. (dues to this seperation the code could be more cleaned up, I dont think keep the methods anymore as they are just a line or two and called only once).

Then, if ignore data changes, run the actual filtering/sorting outside the method execution with a run later of 0, milliseconds. This way the transaction does NOT see the data access and so does not trigger a tracked change.

[cah-brian-gantzler]

The use of localCopy on serveral of the properties will need to be marked as a breaking change.

For Example: on the column component the visible property was defined as

/**
   * Set to `false` to hide the entire column across all rows. Keep in mind that this property
   * won't just hide the column using css. The DOM for the column will be removed. Defaults to `true`.
   */
  visible = true;

This means that true is assigned to the variable during construction, then the arguments are applied. So if nothing was passed in the value would remain true, if undefined was passed in, the value would be undefined. In most cases undefined is treated as false.

It is now defined as

/**
   * Set to `false` to hide the entire column across all rows. Keep in mind that this property
   * won't just hide the column using css. The DOM for the column will be removed. Defaults to `true`.
   *
   * @argument visible
   * @type {Boolean}
   */
  @localCopy('args.visible', true)
  visible;

What this means is that if nothing is passed in it will be initialized to true. However, if undefined is passed in, it is also initialized to true (see https://github.com/tracked-tools/tracked-toolbox/blob/master/tracked-toolbox/src/index.js#L15)

I found this in that I was passing in undefined in some of my code instead of false and tests were breaking. This used to work because undefined was a falsey value. Now localCopy is forcing it to true.

This would be a breaking change for every boolean value using localCopy that has an intializer of true

[cah-brian-gantzler]

Pretty much got this as far as it can go. Need some support if you want to merge my PR into yours.

You had documented some breaking changes, there are more

• Support for ember 3.16+ by refactoring component file structure for converting to glimmer #345
• Removed polyfills no longer needed for supporting 3.16+
• Dropped support for Node 14.
• Removed yielded for head and foot from table api (Previously deprecated)
• cells no longer support the ability to specify visible on the cells themselves. Please set visible on the column
• many booleans passed in if undefined are not treated as true instead of false (ex column.visible)

✨ Features

• Converted components to user glimmer syntax #345 #351

• Converted components to use template imports

• Updated husky/prettier/linters

• switch from npm to pnpm

• Notes:

• is eslint-plugin-template-imports really needed? With template imports this might be a conflict. Removed for now

• the updateFilter and updateTotalRows use modifiers but cant be modifiers as they do not work if you set renderTableElement to false. I write them as plain old functions as helpers. This requires 4.5+. Could rewrite as custom helpers if want to go back farther.

[cah-brian-gantzler]

Dont know how many people are using ember-yeti table, but are there any on ember 4.4 or less? 3.26 or less?

Would like to know if there is any reason to rewrite a couple features to try and support those versions.

Thanks

[cah-brian-gantzler]

Been using my new version, would really like to get it merged here. Been bitten a few times in that a boolean that was undefined use to be treated as false, but not is treated as the default (ie true for column visible...).

But also just got bit by the visible property NOT being on each cell but on column only. If the footer, we had a cell that spanned the entire row, put the pagination control in it.... and if the first column is hidden, not the whole pagination disappears because the cell cant have visible on it.

[AmauryD]

Hello, my team has been using Yeti-Table for quite some time now. I see the issue a bit late but if i can be of any help to fix the bugs to convert this addon to modern ember it will be a pleasure.

[cah-brian-gantzler]

It has been converted completely and I have a PR out for it. Just waiting for it to get merged. In the meantime I have pointed my app to the branch. There are breaking changes.

This is what I have in my package.json

    "ember-yeti-table": "git+https://github.com/bgantzler/ember-yeti-table#helpers",

[cah-brian-gantzler]

I have detected an error in the converted code. The api.rows and api.totalRows are not taking into account the filtered data. Also when you sorted a column, api.rows reflected the sort. This is no longer the case and looking into a fix. Will post with a new PR once I find the solution.

However, havent really heard back about getting this merged so may have to contemplate releasing a new addon if I do not hear back from @miguelcobain

Everything tested out, so a new PR has been posted that fixes this.

[miguelcobain]

@cah-brian-gantzler I'm here! I'm sorry, had some busy weeks.

Nice work on the @ignoreDataChanges.

Some questions/requests:

• regarding the localCopy breaking change you mentioned here, I guess it's hard to distinguish between not passing anything and passing undefined in glimmer components world, right? Is this breaking change avoidable? If it isn't avoidable, can we at least list those boolean properties affected in the CHANGELOG?
• regarding this breaking change

  the updateFilter and updateTotalRows use modifiers but cant be modifiers as they do not work if you set renderTableElement to false. I write them as plain old functions as helpers. This requires 4.5+. Could rewrite as custom helpers if want to go back farther.

As far as I can see, you updated them to use helpers, right? Did you consider using the polyfill for this? Perhaps that would be the best way to do this, since it would allow us to write more modern/simpler code and still be compatible with old versions: https://github.com/ember-polyfills/ember-functions-as-helper-polyfill

• regarding this one

  cells no longer support the ability to specify visible on the cells themselves. Please set visible on the column

Why is this the case?

Great job on everything. Looks like we're almost there.

[cah-brian-gantzler]

• regarding the localCopy breaking change you mentioned here, I guess it's hard to distinguish between not passing anything and passing undefined in glimmer components world, right? Is this breaking change avoidable? If it isn't avoidable, can we at least list those boolean properties affected in the CHANGELOG?

Its any boolean that defaults to true, so yea, will have to get a list

• regarding this breaking change

the updateFilter and updateTotalRows use modifiers but cant be modifiers as they do not work if you set renderTableElement to false. I write them as plain old functions as helpers. This requires 4.5+. Could rewrite as custom helpers if want to go back farther.

As far as I can see, you updated them to use helpers, right? Did you consider using the polyfill for this? Perhaps that would be the best way to do this, since it would allow us to write more modern/simpler code and still be compatible with old versions: https://github.com/ember-polyfills/ember-functions-as-helper-polyfill

Its internal code, and the helpers are not that big a deal, so I left them. Can always change them later. Right now it supports 3.28+. Didnt see the need to add another dependency, but sure. If you want that can do it.

• regarding this one

cells no longer support the ability to specify visible on the cells themselves. Please set visible on the column

Why is this the case?

This was done in the code you provided that I built on, so figured you can your reasons. LOL We actually used it and had to change our code to do it a different way to honor your change. So, if you do not want that anymore, can see about bringing it back.

[miguelcobain]

This was done in the code you provided that I built on, so figured you can your reasons. LOL

Oops. 😅 I totally forgot why that was changed. Could it have been accidental?

[cah-brian-gantzler]

Dont think so, if you look at the change log on your octane branch https://github.com/miguelcobain/ember-yeti-table/blob/octane/CHANGELOG.md you documented it.

It kinda makes sense, either the whole column is visible or its not, having it on the cell would allow you to make a column visible for three rows then off for two etc, kinda weird. However we used in the in total row, the first column did a colspan and put the pagination controls there. Forced the first column to visible in the total row. With the current change, when the first column became invisible, we lost the pagination controls.

Had to fix by not putting the pagination controls in the total row but somewhere else.

[cah-brian-gantzler]

Kinda forgot about this, looks like im still using my own repo. We going to possibly be able to merge this in sometime?

[AmauryD]

Kinda forgot about this, looks like im still using my own repo. We going to possibly be able to merge this in sometime?

Using your repo in one of my company's project. Works like a charm. No bugs encountered.