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

Remove remaining deprecated apis #12

Merged
merged 11 commits into from
Oct 21, 2022

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Oct 11, 2022

Requires: #11

  • Removes deprecated APIs - extracted to chore(cleanup): remove remaining deprecated APIs #24
  • Re-does the typings to have a Signature interface instead of 4+ type arguments to BasePlugin, which is more extensible, and maintainable long term, but has higher upfront cost for figuring out inference - extracted to Introduce plugin signatures #39
  • Creates plugins for the removed deprecated code
    • Row Selection (because rows have a single on-click handler)
    • Row-plugins
      • row-modifier
        • docs
        • tests
      • row meta
        • docs
        • tests

@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2022

@nicolechung
Copy link
Contributor

dark-mode

Unrelated, but already with the dark mode 🎉 !!

@NullVoxPopuli NullVoxPopuli force-pushed the remove-remaining-deprecated-apis branch 4 times, most recently from 308f2b5 to 2911457 Compare October 18, 2022 22:12
@NullVoxPopuli NullVoxPopuli force-pushed the remove-remaining-deprecated-apis branch 2 times, most recently from 7f12f05 to 59487a3 Compare October 19, 2022 23:13
@NullVoxPopuli NullVoxPopuli force-pushed the remove-remaining-deprecated-apis branch from 59487a3 to e46ce50 Compare October 19, 2022 23:43
@nicolechung
Copy link
Contributor

Preview URLs

GH Env: preview docs: https://b81f639d.ember-headless-table.pages.dev api docs: https://b81f639d.ember-headless-table.pages.dev/api/modules.html

It would be nice if this managed to update with new commits.

@@ -46,6 +47,9 @@
"plugins/sticky-columns": [
"./dist/plugins/sticky-columns/index.d.ts"
],
"plugins/row-selection": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something we manually have to add for each new plugin?

Maybe this should be a note in CONTRIBUTING

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's something that needs to be added in any package for any public entry point -- it's more "normal npm stuff", if that makes sense.
We probably should have a pointer to some doc talking about this tho.

@@ -0,0 +1,312 @@
import Component from '@glimmer/component';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the *.gts extension here? Is that this

Asking because I don't see other *.gts` files in the project, wondering if this is a new convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all the rendering tests use gts.

Is that this

nay, it's this: https://rfcs.emberjs.com/id/0779-first-class-component-templates

@NullVoxPopuli
Copy link
Contributor Author

It would be nice if this managed to update with new commits.

it does, but Cloudflare doesn't run as fast as the tests.
image

"@glint/environment-ember-loose": "^0.9.5",
"@glint/environment-ember-template-imports": "^0.9.5",
"@glint/template": "^0.9.5",
"@glint/core": "^0.9.6",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this version fixes some bugs I was working around, so I just upgraded to it

@@ -0,0 +1,12 @@
---
categoryOrder: 3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can now sort folders!

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review October 20, 2022 22:05
@NullVoxPopuli
Copy link
Contributor Author

Finally green 🎉

@nicolechung
Copy link
Contributor

Screen Shot 2022-10-21 at 9 26 25 AM

I can't seem to run pnpm start locally (again). This is after deleting node_modules from all 5 packages, running pnpm install then pnpm build.

@NullVoxPopuli
Copy link
Contributor Author

this is probably where turbo might be useful, we can force the build of the addon before starting the app.
Based on the error, it looks like the addon isn't built yet

@nicolechung
Copy link
Contributor

this is probably where turbo might be useful, we can force the build of the addon before starting the app. Based on the error, it looks like the addon isn't built yet

But isn't this what pnpm build does? I ran this before pnpm start

Screen Shot 2022-10-21 at 9 54 17 AM

Screen Shot 2022-10-21 at 9 54 04 AM

It looks like docs-app is looking into node_modules for ember-headless-table which is in the...pnpm cache but that doesn't have a dist folder, but there is a dist folder in the ember-headless-table package / folder.

@NullVoxPopuli
Copy link
Contributor Author

what version of pnpm are you using? I have 7.13.4, and just upgraded to 7.13.6

@nicolechung
Copy link
Contributor

nicolechung commented Oct 21, 2022

what version of pnpm are you using? I have 7.13.4, and just upgraded to 7.13.6

pnpm --version
7.5.0

Let me update

@nicolechung
Copy link
Contributor

Tried again with

pnpm i
pnpm build
pnpm i
pnpm start

Works for me now.

```hbs template

FPS: {{this.fps}}<br>
<div data-container class="h-full overflow-auto" {{this.table.modifiers.container}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

come on, github. where's your syntax highlighting... :(

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, the demo doesn't have it either:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I noticed that, too. but that's likely my fault.

GitHub requires that at least 200 repos use a new syntax before they add highlighting for it -- and I don't think they've ever supported nested highlighting in markdown without "rendering the file"

docs/demos/lots-of-data/index.md Outdated Show resolved Hide resolved

import { headlessTable } from 'ember-headless-table';
import { RowSelection, toggle, isSelected, select, deselect } from 'ember-headless-table/plugins/row-selection';
import { DATA } from 'test-app/data';
Copy link
Contributor

Choose a reason for hiding this comment

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

Again with the lack of syntax highlighting here :(

This one's pretty funny, though:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh! that is goofy!

assert.expect(1);

setupOnerror((error) => {
let errorStr = error instanceof Error ? error.message : `${error}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I usually prefer String(error) over ${error}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why's that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to go by https://eslint.org/docs/latest/rules/no-implicit-coercion

To me, it's more clear that you're intending to coerce to a string if you say String(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

legit, is it available in Eslint 7?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup!

This rule was introduced in ESLint v1.0.0-rc-2.

await click(rows[0]);

assert.strictEqual(ctx.selection.size, 0);
assert.deepEqual([...ctx.selection.values()], []);
Copy link
Contributor

Choose a reason for hiding this comment

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

With so many assertions here, would it help to add failure messages? Otherwise it may be hard to keep track of what's failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe -- I didn't have any issue debugging this when I wrote it though -- the contents of expected and actual are printed in the browser -- so it's fairly easy to see what's going on, imo.
(obvs, if I get confused later, messages will get added 😅 )

@NullVoxPopuli NullVoxPopuli merged commit 6ba902f into main Oct 21, 2022
@NullVoxPopuli NullVoxPopuli deleted the remove-remaining-deprecated-apis branch October 21, 2022 22:12
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants