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: Upgrade through 43 minor versions, 2 majors, and 2 paradigm shifts #2209

Merged
merged 24 commits into from
Dec 18, 2023

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Sep 30, 2023

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? 😅
(like, do I need super old node?)

@NullVoxPopuli NullVoxPopuli changed the title WIP: Ember: Upgrade through 41 minor versions, 2 majors, and 2 paradigm shifts Ember: Upgrade through 41 minor versions, 2 majors, and 2 paradigm shifts Oct 1, 2023
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review October 1, 2023 01:59
@a13o
Copy link

a13o commented Oct 2, 2023

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
Copy link
Contributor Author

NullVoxPopuli commented Oct 2, 2023

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

Issue report here: nodejs/node#49911

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

@a13o
Copy link

a13o commented Oct 3, 2023

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
Copy link
Contributor Author

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.
image
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
Copy link
Contributor Author

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

@NullVoxPopuli
Copy link
Contributor Author

I added some tests!

@NullVoxPopuli
Copy link
Contributor Author

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

Copy link
Collaborator

@flashdesignory flashdesignory left a comment

Choose a reason for hiding this comment

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

I wasn't able to run the standalone version linked in the comments.
Screenshot 2023-12-05 at 3 36 25 PM


@service repo;

// TODO: we should use a <form> instead of this.
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, this would be a larger refactor though, since the template as well as the styles package would need to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but it would be a correct change

Copy link
Collaborator

Choose a reason for hiding this comment

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

absolutely!
@FadySamirSadek - I know we talked about a11y changes (globally).
Is there already an issue filed? If not, I can file one in the next few days ..

</footer>

{{!--
Looks like the latest todoMVC styles are broken?
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: what's broken? I think as long as the app looks consistent with the other apps it should be ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk, I'll remove this comment -- it's been too long

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Dec 5, 2023

@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
Copy link
Collaborator

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
Copy link
Collaborator

@NullVoxPopuli - app looks great and your instructions helped!

Found one tiny bug:
output

@NullVoxPopuli
Copy link
Contributor Author

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

@flashdesignory
Copy link
Collaborator

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:

  1. if there are no active todos in the app, the toggle all should be checked
  2. unchecking the toggle all should remove the items from the completed view (since then the items would not be completed)
  3. 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 NullVoxPopuli changed the title Ember: Upgrade through 41 minor versions, 2 majors, and 2 paradigm shifts Ember: Upgrade through 43 minor versions, 2 majors, and 2 paradigm shifts Dec 12, 2023
@NullVoxPopuli
Copy link
Contributor Author

@flashdesignory thanks!

I believe I've fixed it now

@flashdesignory flashdesignory merged commit 5bb801a into tastejs:master Dec 18, 2023
@NullVoxPopuli NullVoxPopuli deleted the upgrade-ember branch December 18, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants