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

Upgrade to glimmer components and native classes #514

Merged
merged 18 commits into from
Sep 24, 2021
Merged

Upgrade to glimmer components and native classes #514

merged 18 commits into from
Sep 24, 2021

Conversation

gilest
Copy link
Collaborator

@gilest gilest commented Sep 1, 2021

Migration to Octane edition. Deals with all legacy ember objects, components, mixins and general tech debt.

Attempting to support the existing public API so that we can release this as a v4.x.0 minor. Possibly with a beta first to make sure there are no regressions.

Notable changes

Queue instances and the FileQueue service

Reworked this relationship significantly.

Previously File objects were added to the FileQueue service as well as the Queue instances themselves. They shared some behaviour for this via the WithFiles mixin.

Now the FileQueue service declaratively accesses files within all Queue instances to provide its public API. All queue flushing has been removed from the FileQueue service – it's the responsibility of the Queue instances only.

Queue flushing

After refactoring away from observers (#307) we added manual calls to Queue.flush() after any internal file state changes.

This required that we warn users they may need to flush manually if they change the file state property themselves:

Implementers should call flush() on the file's queue after mutating this property.

I've refactored this by using a setter to call Queue.flush() on any file state changes and removed this piece of documentation.

Queue instances and Components

When FileUpload/FileDropzone call didRecieveAttrs they directly set some properties on their queue.

I've refactored this to an update-queue modifier.

  {{update-queue
    this.queue
    accept=@accept
    multiple=@multiple
    disabled=@disabled
    onFileAdd=this.onFileAdd
  }}

When testing this I noticed that it's an unsafe operation – the queue only has a single property for each of these and there's no guarantee about the order in which properties might overwrite each other. (For example when rendering a dropzone and a file select for the same queue).

I'm comfortable releasing as-is for now (believe it's the same as existing functionality) – but we should reconsider this API in the future.

DragListener

I've also refactored this into a modifier.

  {{drag-listener
    dragenter=this.didEnterDropzone
    dragleave=this.didLeaveDropzone
    dragover=this.didDragOver
    drop=this.didDrop
  }}

This required me to adjust the DragListener class to work with an element rather than a selector.

Test waiters

Migrated from old registerWaiter to the new buildWaiter from @ember/test-waiters.

Component DOM

Almost identical to before. Since FileDropzone renders a wrapper div I added ...attributes to it.

FileUpload is a bit more complex with the label/input relationship and existing attribute args so I left it the same as before.

I also removed an assertion about the wrapper tag of FileUpload as it's no longer possible to pass in tagName and override it.

Dependencies

Since I added modifiers I've added ember-modifier to dependencies.

Also added glimmer packages and removed @babel/core - not sure why it was ever included.

Questions

  • Have I set up the modifiers properly? They are only for internal use.
  • Are there any dependencies I've failed to add?
  • I've used optional chaining ?. and nullish coalescing ??. Is this safe? Do I need to configure something for this?
  • I added a dir addon/utils is that safe? Will that work?
  • Any properties I forgot to track?

@gilest gilest changed the title Octane WIP – Octane Sep 1, 2021
@gilest gilest requested a review from jelhan September 3, 2021 01:59
@gilest gilest changed the title WIP – Octane Octane Sep 3, 2021
@gilest gilest marked this pull request as ready for review September 3, 2021 02:05
@gilest gilest changed the title Octane Upgrade to glimmer components and native classes Sep 3, 2021
addon/file.js Outdated Show resolved Hide resolved
addon/services/file-queue.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
…es mixin

Derive all files at the top level from child queue state
Only flush files at Queue level

wip - test waiters
- add glimmer packages
- remove @babel/core as it is a child dependency of ember-cli-babel
Ensure queue is updated when component args update by implementing update-queue modifer.
More similar behaviour to previous classic component version.
Migrated remaining usage of .extend to native classes.
Move `@ember/test-waiters` to dependencies
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.

2 participants