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

refactor: remove jq dependency #35

Merged
merged 1 commit into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/nodejs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:

strategy:
matrix:
node-version: [14.x]
node-version: [20.x]

steps:
- uses: actions/checkout@v2
Expand Down
2 changes: 1 addition & 1 deletion .nvmrc
Original file line number Diff line number Diff line change
@@ -1 +1 @@
16.15.0
20.9.0
2 changes: 1 addition & 1 deletion .tool-versions
Original file line number Diff line number Diff line change
@@ -1 +1 @@
nodejs 14.17.1
nodejs 20.9.0
5 changes: 1 addition & 4 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@g2crowd/widget",
"version": "2.0.0",
"description": "Rails-friendly jquery plugin wrapper",
"description": "Rails-friendly plugin wrapper",
"repository": "https://github.com/g2crowd/widget",
"author": "Michael Wheeler",
"license": "MIT",
Expand All @@ -21,9 +21,6 @@
"jest": "^27.0.5",
"prettier": "^2.3.0"
},
"peerDependencies": {
"jquery": ">=1.6.2"
},
"module": "src/index.js",
"main": "src/index.js",
"files": [
Expand Down
6 changes: 3 additions & 3 deletions src/initWidgets.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import queue from './queue';
import { extractOptions } from '@g2crowd/extract-options';
import camelize from './camelize';
import { strategies } from './strategies';
import * as strategies from './strategies';

function simpleCaste(value) {
try {
Expand Down Expand Up @@ -50,10 +50,10 @@ const loadWidget = function (element, name, widgetQueue, registered) {

if (!existingPlugin) {
widgetQueue.add(() => {
strategies.get(pluginFn.init)(wrapped, element);
const initStrategy = strategies[pluginFn.init] || strategies.fallback(pluginFn.init, strategies.nextTick);
initStrategy(wrapped, element);
});
widgetQueue.flush();

element.dataset[`vvidget_${camelize(name)}`] = true;
}
};
Expand Down
13 changes: 0 additions & 13 deletions src/initiationStrategies.js

This file was deleted.

31 changes: 16 additions & 15 deletions src/strategies.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
import initiationStrategies from './initiationStrategies';

export const strategies = initiationStrategies({
nextTick(pluginFn, $$) {
return window.setTimeout(() => pluginFn(), 0);
},

immediate(pluginFn, $$) {
return pluginFn() || {};
},

hover(pluginFn, $$) {
return $$.one('mouseover', () => pluginFn());
}
});
export function nextTick(pluginFn, _el) {
return window.setTimeout(() => pluginFn(), 0);
}
export function immediate(pluginFn, _el) {
return pluginFn() || {};
}
export function hover(pluginFn, el) {
const isJqueryEl = el.jquery !== undefined;
return isJqueryEl ?
Copy link
Member

Choose a reason for hiding this comment

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

Can we just leave it up to the the client code to add a new hover strategy with widget.strategies.add('hover', () => {}), if they want to have a jQuery hover option?

Copy link
Member

Choose a reason for hiding this comment

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

In fact I don't think this works, since #31 the hover strategy has been broken.

Notice that in UE we're using 1.3.0. We use 2.0.0 in another project and haven't used the hover strategy in that project yet.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, still keeping the registry and opening it up to client registering new strategies makes sense.

el.one('mouseover', pluginFn) :
el.addEventListener('mouseover', pluginFn, { once: true });
}
export function fallback(strategy, fallback) {
console.warn(`Strategy ${strategy} not found, falling back to ${fallback.name} strategy.`);
return fallback;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just throw InitiationNotSupportedError? This is a major mistake, it doesn't feel like a warning is sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

So then not fallback at all.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's what I prefer anyway. I don't think throwing a "function undefined" method is helpful, but having a strict interface is good too.

Copy link
Member

@wheeyls wheeyls Feb 24, 2024

Choose a reason for hiding this comment

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

Actually I'm wrong, the fallback with warning is a good idea.

Moving a widget into a different environment probably shouldn't break it.

Copy link
Author

Choose a reason for hiding this comment

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

Actually I'm wrong, the fallback with warning is a good idea.

Moving a widget into a different environment probably shouldn't break it.

Yeah, that was my line too 👍

}
19 changes: 0 additions & 19 deletions test/initiationStrategies.test.js

This file was deleted.

56 changes: 56 additions & 0 deletions test/strategies.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@

import * as strategies from '../src/strategies';
jest.useFakeTimers();

describe('strategies', () => {
beforeEach(() => {
jest.clearAllTimers();
});
describe('fallback', () => {
it('should fall back to the provided strategy', () => {
const fallback = strategies.fallback('nonexistent', strategies.nextTick);
expect(fallback).toEqual(strategies.nextTick);
});

it('should log a warning', () => {
const nonexistentStrategy = 'nonexistent';
const fallbackStrategy = strategies.nextTick;
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => { });
strategies.fallback(nonexistentStrategy, fallbackStrategy);
expect(warnSpy).toHaveBeenCalledWith(`Strategy ${nonexistentStrategy} not found, falling back to ${fallbackStrategy.name} strategy.`);
});
})
describe('nextTick', () => {
it('should call the plugin function', () => {
const pluginFn = jest.fn();
strategies.nextTick(pluginFn);
jest.runAllTimers();
expect(pluginFn).toHaveBeenCalled();
});
});
describe('immediate', () => {
it('should call the plugin function', () => {
const pluginFn = jest.fn();
strategies.immediate(pluginFn);
expect(pluginFn).toHaveBeenCalled();
});
});
describe('hover', () => {
describe('with a jQuery element', () => {
it('should call the plugin function', () => {
const pluginFn = jest.fn();
const el = { jquery: true, one: jest.fn() };
strategies.hover(pluginFn, el);
expect(el.one).toHaveBeenCalledWith('mouseover', pluginFn);
});
});
describe('with a DOM element', () => {
it('should call the plugin function', () => {
const pluginFn = jest.fn();
const el = { addEventListener: jest.fn() };
strategies.hover(pluginFn, el);
expect(el.addEventListener).toHaveBeenCalledWith('mouseover', pluginFn, { once: true });
});
})
});
})
Loading