Skip to content

Update to new modifier apis (modify) instead of lifecycles #57

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.

Sign up for GitHub

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

Open
betocantu93 wants to merge 3 commits into
base: gen-4
Choose a base branch
from
Open

Update to new modifier apis (modify) instead of lifecycles #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6232aa9
BREAKING: upgrade to ember-modifiers 4.x
betocantu93 Oct 22, 2023
0a3df8b
bump node
betocantu93 Oct 22, 2023
81be862
fix: registerDestructor doesn't bind the context
betocantu93 Oct 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 31 additions & 29 deletions addon/-private/modifier.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import {
Sizes,
SIZES_DEFAULT,
SIZES_HEIGHT_DEFAULT,
Args,
} from 'ember-element-query';
import { registerDestructor } from '@ember/destroyable';
import { inject as service } from '@ember/service';
export interface SizeObject {
name: string;
Expand Down Expand Up @@ -41,9 +43,11 @@ export default class ElementQueryModifier extends Modifier<ModifierArgs> {
sizesHeightDefault: Sizes = SIZES_HEIGHT_DEFAULT;
sizesRatioDefault: Sizes = SIZES_RATIO_DEFAULT;

_element?: HTMLElement; // For some reason, this.element is not always available
_element?: HTMLElement; // For some reason, this._element is not always available
Copy link
Owner

@lolmaus lolmaus Oct 23, 2023

Choose a reason for hiding this comment

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

This is wrong, please revert.

_firstCall = true;

named: Args = {};

// -------------------
// Computed properties
// -------------------
Expand All @@ -59,7 +63,7 @@ export default class ElementQueryModifier extends Modifier<ModifierArgs> {
}

get debounce(): number {
const { debounce } = this.args.named;
const { debounce } = this.named;

return debounce == null ? 100 : debounce;
}
Expand All @@ -69,7 +73,7 @@ export default class ElementQueryModifier extends Modifier<ModifierArgs> {
}

get sizes(): Sizes | undefined {
const { sizes } = this.args.named;
const { sizes } = this.named;

// Explicitly opt into default sizes
if (sizes === true) {
Expand All @@ -93,7 +97,7 @@ export default class ElementQueryModifier extends Modifier<ModifierArgs> {
}

get sizesHeight(): Sizes | undefined {
const { sizesHeight } = this.args.named;
const { sizesHeight } = this.named;

// Explicitly opt into default sizes
if (sizesHeight === true) {
Expand All @@ -112,7 +116,7 @@ export default class ElementQueryModifier extends Modifier<ModifierArgs> {
}

get sizesRatio(): Sizes | undefined {
const { sizesRatio } = this.args.named;
const { sizesRatio } = this.named;

// Explicitly opt into default sizes
if (sizesRatio === true) {
Expand Down Expand Up @@ -461,7 +465,7 @@ export default class ElementQueryModifier extends Modifier<ModifierArgs> {
size: this.sizeObjectWidthAt?.name,
sizeHeight: this.sizeObjectHeightAt?.name,
sizeRatio: this.sizeObjectRatioAt?.name,
prefix: this.args.named.prefix,
prefix: this.named.prefix,
attributes: this.attributes,
attributesRecord: this.attributesRecord,
};
Expand All @@ -472,7 +476,7 @@ export default class ElementQueryModifier extends Modifier<ModifierArgs> {
// -------------------

convertSizeToAttribute(size: string, rangeDirection: RangeDirection): string {
const prefix = this.args.named.prefix ?? '';
const prefix = this.named.prefix ?? '';

if (typeof prefix !== 'string') {
throw new Error(`element-query: expected prefix to be a string, was ${typeof prefix}`);
Expand All @@ -497,8 +501,8 @@ export default class ElementQueryModifier extends Modifier<ModifierArgs> {
}

callOnResize(): void {
if (this.args.named.onResize) {
this.args.named.onResize(this.yield);
if (this.named.onResize) {
this.named.onResize(this.yield);
}
}

Expand Down Expand Up @@ -544,7 +548,7 @@ export default class ElementQueryModifier extends Modifier<ModifierArgs> {
}

_didResizeHandlerSync(): void {
if (!this.args.named.isDisabled && !this.isDestroying && !this.isDestroyed) {
if (!this.named.isDisabled && !this.isDestroying && !this.isDestroyed) {
this.applyAttributesToElement();
this.callOnResize();
}
Expand All @@ -564,28 +568,26 @@ export default class ElementQueryModifier extends Modifier<ModifierArgs> {
debounceTask(this, '_didResizeHandlerRequestAnimationFrame', this.debounce);
}

// -------------------
// Lifecycle hooks
// -------------------
didInstall(): void {
if (!this.element) throw new Error('Expected this.element to be available');

this._element = this.element;

this.resizeObserver.observe(this.element, this.didResizeHandler); // eslint-disable-line @typescript-eslint/unbound-method

this._didResizeHandlerSync(); // We want to run it as soon as possible to avoid a jump of unstyled content
}

didUpdateArguments(): void {
if (!this.resizeObserver.isEnabled) return;
modify(element: HTMLElement, _pos: unknown[], named: Args): void {
this._element = element;
this.named = named;
if (this._firstCall) {
// eslint-disable-next-line @typescript-eslint/unbound-method
this.resizeObserver.observe(element, this.didResizeHandler);
this._didResizeHandlerSync();

this.didResizeHandler();
registerDestructor(this, () => this.cleanup());
Copy link
Owner

@lolmaus lolmaus Oct 23, 2023

Choose a reason for hiding this comment

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

Why this change (arrow function)? In order to remove the lint exception? But we have this exact lint exception used all over the file, so I don't see a point in fixing just one instance.

The linter should serve to the developer, not vice versa. :)

this._firstCall = false;
} else {
if (this.resizeObserver.isEnabled) {
this.didResizeHandler();
}
}
}

willRemove(): void {
if (!this.element) throw new Error('Expected this.element to be available');
cleanup(): void {
if (!this._element) throw new Error('Expected this._element to be available');

this.resizeObserver.unobserve(this.element, this.didResizeHandler); // eslint-disable-line @typescript-eslint/unbound-method
this.resizeObserver.unobserve(this._element, this.didResizeHandler); // eslint-disable-line @typescript-eslint/unbound-method
}
}
8 changes: 4 additions & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@
"ember-cli-babel": "^7.26.3",
"ember-cli-htmlbars": "5.7.1",
"ember-cli-typescript": "^4.2.1",
"ember-element-helper": "^0.5.5",
"ember-element-helper": "^0.8.5",
"ember-lifeline": "^6.0.2",
"ember-modifier": "^2.1.2",
"ember-modifier": "^4.1.0",
"ember-resize-observer-service": "^0.3.0",
"ember-window-mock": "^0.7.2"
},
Expand Down Expand Up @@ -146,7 +146,7 @@
"webpack": "^5.70.0"
},
"engines": {
"node": "10.* || >= 12"
"node": "14.* || 16.* || >= 18"
},
"publishConfig": {
"registry": "https://registry.npmjs.org"
Expand Down Expand Up @@ -174,7 +174,7 @@
}
},
"volta": {
"node": "12.18.0",
"node": "16.17.0",
"yarn": "1.22.4"
}
}
Loading