Ember: Upgrade through 43 minor versions, 2 majors, and 2 paradigm shifts

tastejs / todomvc

Helping you select an MV* framework - Todo apps for React.js, Ember.js, Angular, and many more

http://todomvc.com

Other

28.6k stars 13.78k forks source link

Ember: Upgrade through 43 minor versions, 2 majors, and 2 paradigm shifts #2209

Closed NullVoxPopuli closed 9 months ago

NullVoxPopuli commented 1 year ago

Updates the ember version of the todo app to the latest ember conventions.

uses polaris features
utilizes focused component files
still uses routing for choosing between all/active/completed (as routing is a core feature of ember)
persists an array of todos to localStorage

there are some a11y issues, like a lack of <form> when we have form-like behavior, but I don't know how much flexibility the provided CSS gives us

I tried to run the tests according to these docs: https://github.com/tastejs/todomvc/tree/master/tests

but I got this error:

❯ npm run test-server

> todomvc@0.1.2 test-server
> gulp test-server &

❯ fs.js:42
} = primordials;
    ^

ReferenceError: primordials is not defined

did I do something wrong? as todomvc too out of date? :sweat_smile: (like, do I need super old node?)

a13o commented 1 year ago

I was trying to run this in a codespace but the build keeps crashing. Either Webpack hangs forever

building... [@embroider/webpack]

or it eventually spits this out

SyntaxError: Bad control character in string literal in JSON at position 182711
    at JSON.parse (<anonymous>)
    at /workspaces/todomvc/examples/emberjs/todomvc/node_modules/.pnpm/thread-loader@3.0.4_webpack@5.88.2/node_modules/thread-loader/dist/WorkerPool.js:144:30
    at Socket.onChunk (/workspaces/todomvc/examples/emberjs/todomvc/node_modules/.pnpm/thread-loader@3.0.4_webpack@5.88.2/node_modules/thread-loader/dist/readBuffer.js:40:9)
    at Socket.emit (node:events:514:28)
    at addChunk (node:internal/streams/readable:343:12)
    at readableAddChunk (node:internal/streams/readable:316:9)
    at Readable.push (node:internal/streams/readable:253:10)
    at Pipe.onStreamRead (node:internal/stream_base_commons:190:23)

NullVoxPopuli commented 1 year ago

If you use node 20 or node 18.18, you'll have that issue.

Issue report here: https://github.com/nodejs/node/issues/49911

If you use node > 16 && < 18.18, you'll have success. If using volta, this'll be chosen for you

a13o commented 1 year ago

I was messing with this tonight to test out the Firefox framework detector stuff. Here's some issues I found

the sourcemaps currently contain a backburner.js.js with duplicate extensions, but the detector looks for /backburner\.js$/ only. does polaris use this name, or is it a config issue in this project?
double clicking doesn't let me edit a todo-item
toggle-all calls repo.save() but i think it wants to call repo.persist(). the change doesn't survive a refresh

NullVoxPopuli commented 1 year ago

does polaris use this name, or is it a config issue in this project?

its unrelated to Polaris, backburner.js, on disk, does actually have .js twice. it's weird. but I think older versions did not? Unsure.

double clicking doesn't let me edit a todo-item toggle-all calls

Is there a shared test suite for all the implementations?

I think that would help

NullVoxPopuli commented 11 months ago

@tcjr any other changes you'd like to see?

NullVoxPopuli commented 11 months ago

I added some tests!

NullVoxPopuli commented 11 months ago

Here is the actual implementation, removing all the boilerplate, if folks want to have something a bit more reviewable: https://github.com/NullVoxPopuli/todomvc-ember-polaris-tests-only/pull/1

NullVoxPopuli commented 10 months ago

@flashdesignory

did you update my remote? the work is done

git log should show you:

* 4f3696ac - fix another bug                                                                                     
|               (7 weeks ago) <NullVoxPopuli>
|               HEAD -> upgrade-ember, origin/upgrade-ember

to run:

cd examples/emberjs/todomvc
pnpm i
pnpm start # dev mode

to build for production

cd examples/emberjs/todomvc
pnpm i
rm -rf dist
pnpm build:production # this is what's committed

I've removed a comment that I didn't have enough context on, so your git log should show

* eef5a421 - Remove comment and rebuild                                                                          
|               (16 seconds ago) <NullVoxPopuli>
|               HEAD -> upgrade-ember, origin/upgrade-ember

To run the production / built assets that are pushed in the dist directory

cd examples/emberjs/todomvc/dist
npx http-server # visit http://localhost:8080

flashdesignory commented 10 months ago

thanks @NullVoxPopuli for the instructions on how to run! 🙏 I was testing the standalone pr, since that's more isolated. I'll try this branch with your instructions

flashdesignory commented 9 months ago

@NullVoxPopuli - app looks great and your instructions helped!

Found one tiny bug: output

NullVoxPopuli commented 9 months ago

what's supposed to be the behavior when clicking the toggle-all button when on the completed view?

flashdesignory commented 9 months ago

I think the list of displayed todo items should update. I'm not sure if there's an official behavior, but this is what I've been using for the completed screen:

if there are no active todos in the app, the toggle all should be checked
unchecking the toggle all should remove the items from the completed view (since then the items would not be completed)
if there are no items to display in the current view, the toggle should be disabled (maybe personal preference, but I found it confusing if all the sudden todo items appear).

This is maybe just my opinion and not necessarily the correct one 🤷

NullVoxPopuli commented 9 months ago

@flashdesignory thanks!

I believe I've fixed it now