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

patch(): node build follow up #8652

Merged
merged 27 commits into from
Feb 5, 2023
Merged
Show file tree
Hide file tree
Changes from 16 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
6 changes: 2 additions & 4 deletions fabric.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ import { isTouchEvent, getPointer } from './src/util/dom_event';
import {
// getScrollLeftTop,
getElementOffset,
cleanUpJsdomNode,
makeElementUnselectable,
makeElementSelectable,
} from './src/util/dom_misc';
Expand Down Expand Up @@ -236,7 +235,6 @@ const util = {
isTouchEvent,
getPointer,
// getScrollLeftTop,
cleanUpJsdomNode,
makeElementUnselectable,
makeElementSelectable,
isTransparent,
Expand Down Expand Up @@ -355,7 +353,7 @@ import { Textbox } from './src/shapes/Textbox';
import { Group } from './src/shapes/Group';
import { ActiveSelection } from './src/shapes/ActiveSelection';
import { Image } from './src/shapes/Image';
import { getEnv, getDocument, getWindow, setEnvForTests } from './src/env';
import { getEnv, getDocument, getWindow, setEnv } from './src/env';
import { createCollectionMixin } from './src/Collection';
import { parseAttributes } from './src/parser/parseAttributes';
import { parseElements } from './src/parser/parseElements';
Expand Down Expand Up @@ -418,7 +416,7 @@ export {
getEnv,
getDocument,
getWindow,
setEnvForTests,
setEnv,
parseStyleAttribute,
parsePointsAttribute,
parseFontDeclaration,
Expand Down
11 changes: 3 additions & 8 deletions index.node.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import './src/env/node';
// first we set the env variable by importing the node env file
import { getNodeCanvas } from './src/env/node';

import type { Canvas as NodeCanvas, JpegConfig, PngConfig } from 'canvas';
import type { JpegConfig, PngConfig } from 'canvas';
import {
Canvas as CanvasBase,
getEnv,
StaticCanvas as StaticCanvasBase,
} from './fabric';
import { FabricObject } from './src/shapes/Object/Object';
Expand All @@ -13,11 +13,6 @@ FabricObject.prototype.objectCaching = false;

export * from './fabric';

function getNodeCanvas(canvasEl: HTMLCanvasElement) {
const impl = getEnv().jsdomImplForWrapper(canvasEl);
return (impl._canvas || impl._image) as NodeCanvas;
}

export class StaticCanvas extends StaticCanvasBase {
getNodeCanvas() {
return getNodeCanvas(this.lowerCanvasEl);
Expand Down
7 changes: 1 addition & 6 deletions rollup.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,6 @@ export default [
},
],
plugins,
external: [
'jsdom',
'jsdom/lib/jsdom/living/generated/utils.js',
'jsdom/lib/jsdom/utils.js',
'canvas',
],
external: ['jsdom', 'jsdom/lib/jsdom/living/generated/utils.js', 'canvas'],
},
];
29 changes: 14 additions & 15 deletions src/canvas/Canvas.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { LEFT_CLICK, MIDDLE_CLICK, RIGHT_CLICK } from '../constants';
import { getEnv } from '../env';
import { getDocument, getWindow } from '../env';
import {
CanvasEvents,
DragEventData,
Expand All @@ -15,7 +15,6 @@ import { Group } from '../shapes/Group';
import type { FabricObject } from '../shapes/Object/FabricObject';
import { AssertKeys } from '../typedefs';
import { isTouchEvent, stopEvent } from '../util/dom_event';
import { createCanvasElement } from '../util/misc/dom';
import { sendPointToPlane } from '../util/misc/planeChange';
import {
isActiveSelection,
Expand Down Expand Up @@ -175,7 +174,7 @@ export class Canvas extends SelectableCanvas {
addOrRemove(functor: any, eventjsFunctor: 'add' | 'remove') {
const canvasElement = this.upperCanvasEl,
eventTypePrefix = this._getEventPrefix();
functor(getEnv().window, 'resize', this._onResize);
functor(getWindow(), 'resize', this._onResize);
functor(canvasElement, eventTypePrefix + 'down', this._onMouseDown);
functor(
canvasElement,
Expand Down Expand Up @@ -218,24 +217,24 @@ export class Canvas extends SelectableCanvas {
// if you dispose on a mouseDown, before mouse up, you need to clean document to...
const eventTypePrefix = this._getEventPrefix();
removeListener(
getEnv().document,
getDocument(),
`${eventTypePrefix}up`,
this._onMouseUp as EventListener
);
removeListener(
getEnv().document,
getDocument(),
'touchend',
this._onTouchEnd as EventListener,
addEventOptions
);
removeListener(
getEnv().document,
getDocument(),
`${eventTypePrefix}move`,
this._onMouseMove as EventListener,
addEventOptions
);
removeListener(
getEnv().document,
getDocument(),
'touchmove',
this._onMouseMove as EventListener,
addEventOptions
Expand Down Expand Up @@ -625,13 +624,13 @@ export class Canvas extends SelectableCanvas {
const canvasElement = this.upperCanvasEl,
eventTypePrefix = this._getEventPrefix();
addListener(
getEnv().document,
getDocument(),
'touchend',
this._onTouchEnd as EventListener,
addEventOptions
);
addListener(
getEnv().document,
getDocument(),
'touchmove',
this._onMouseMove as EventListener,
addEventOptions
Expand Down Expand Up @@ -660,12 +659,12 @@ export class Canvas extends SelectableCanvas {
addEventOptions
);
addListener(
getEnv().document,
getDocument(),
`${eventTypePrefix}up`,
this._onMouseUp as EventListener
);
addListener(
getEnv().document,
getDocument(),
`${eventTypePrefix}move`,
this._onMouseMove as EventListener,
addEventOptions
Expand All @@ -686,13 +685,13 @@ export class Canvas extends SelectableCanvas {
this.mainTouchId = null;
const eventTypePrefix = this._getEventPrefix();
removeListener(
getEnv().document,
getDocument(),
'touchend',
this._onTouchEnd as EventListener,
addEventOptions
);
removeListener(
getEnv().document,
getDocument(),
'touchmove',
this._onMouseMove as EventListener,
addEventOptions
Expand Down Expand Up @@ -723,12 +722,12 @@ export class Canvas extends SelectableCanvas {
eventTypePrefix = this._getEventPrefix();
if (this._isMainEvent(e)) {
removeListener(
getEnv().document,
getDocument(),
`${eventTypePrefix}up`,
this._onMouseUp as EventListener
);
removeListener(
getEnv().document,
getDocument(),
`${eventTypePrefix}move`,
this._onMouseMove as EventListener,
addEventOptions
Expand Down
14 changes: 5 additions & 9 deletions src/canvas/SelectableCanvas.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getEnv } from '../env';
import { getDocument, getEnv } from '../env';
import { dragHandler } from '../controls/drag';
import { getActionFromCorner } from '../controls/util';
import { Point } from '../Point';
Expand Down Expand Up @@ -26,11 +26,7 @@ import { TMat2D, TOriginX, TOriginY, TSize } from '../typedefs';
import { degreesToRadians } from '../util/misc/radiansDegreesConversion';
import { getPointer, isTouchEvent } from '../util/dom_event';
import type { IText } from '../shapes/IText/IText';
import {
cleanUpJsdomNode,
makeElementUnselectable,
wrapElement,
} from '../util/dom_misc';
import { makeElementUnselectable, wrapElement } from '../util/dom_misc';
import { setStyle } from '../util/dom_style';
import type { BaseBrush } from '../brushes/BaseBrush';
import { pick } from '../util/misc/pick';
Expand Down Expand Up @@ -1241,7 +1237,7 @@ export class SelectableCanvas<
}

protected _initWrapperElement() {
const container = getEnv().document.createElement('div');
const container = getDocument().createElement('div');
container.classList.add(this.containerClass);
this.wrapperEl = wrapElement(this.lowerCanvasEl, container);
this.wrapperEl.setAttribute('data-fabric', 'wrapper');
Expand Down Expand Up @@ -1504,9 +1500,9 @@ export class SelectableCanvas<
this.contextCache = null;
this.contextTop = null;
// TODO: interactive canvas should NOT be used in node, therefore there is no reason to cleanup node canvas
cleanUpJsdomNode(upperCanvasEl);
getEnv().dispose(upperCanvasEl);
this.upperCanvasEl = undefined;
cleanUpJsdomNode(cacheCanvasEl);
getEnv().dispose(cacheCanvasEl);
this.cacheCanvasEl = undefined;
if (wrapperEl.parentNode) {
wrapperEl.parentNode.replaceChild(lowerCanvasEl, wrapperEl);
Expand Down
8 changes: 4 additions & 4 deletions src/canvas/StaticCanvas.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getEnv } from '../env';
import { getDocument, getEnv } from '../env';
import { config } from '../config';
import { iMatrix, VERSION } from '../constants';
import type { CanvasEvents, StaticCanvasEvents } from '../EventTypeDefs';
Expand Down Expand Up @@ -26,7 +26,7 @@ import {
cancelAnimFrame,
requestAnimFrame,
} from '../util/animation/AnimationFrameProvider';
import { cleanUpJsdomNode, getElementOffset } from '../util/dom_misc';
import { getElementOffset } from '../util/dom_misc';
import { uid } from '../util/internals/uid';
import { createCanvasElement, isHTMLCanvas, toDataURL } from '../util/misc/dom';
import { invertTransform, transformPoint } from '../util/misc/matrix';
Expand Down Expand Up @@ -397,7 +397,7 @@ export class StaticCanvas<
this.lowerCanvasEl = canvasEl;
} else {
this.lowerCanvasEl =
(getEnv().document.getElementById(canvasEl) as HTMLCanvasElement) ||
(getDocument().getElementById(canvasEl) as HTMLCanvasElement) ||
this._createCanvasElement();
}
if (this.lowerCanvasEl.hasAttribute('data-fabric')) {
Expand Down Expand Up @@ -1675,7 +1675,7 @@ export class StaticCanvas<
canvasElement.setAttribute('height', `${this.height}`);
canvasElement.style.cssText = this._originalCanvasStyle || '';
this._originalCanvasStyle = undefined;
cleanUpJsdomNode(canvasElement);
getEnv().dispose(canvasElement);
}

/**
Expand Down
6 changes: 5 additions & 1 deletion src/env/browser.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* eslint-disable no-restricted-globals */
import { config } from '../config';
import { WebGLProbe } from '../filters/GLProbes/WebGLProbe';
import { TCopyPasteData, TFabricEnv } from './types';

const copyPasteData: TCopyPasteData = {};
Expand All @@ -22,7 +23,10 @@ export const getEnv = (): TFabricEnv => {
document,
window,
isTouchSupported,
isLikelyNode: false,
GLProbe: new WebGLProbe(),
dispose() {
// noop
},
copyPasteData,
};
};
14 changes: 9 additions & 5 deletions src/env/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
/**
* This file is consumed by fabric.
* The `./node` and `./browser` files define the env variable that is used by this module.
* The `./node` module sets the env at import time.
* The `./browser` module is defined to be the default env and doesn't set the env at all.
* This is done in order to support SSR.
ShaMan123 marked this conversation as resolved.
Show resolved Hide resolved
* Since window and document aren't defined at time of import in SSR, we can't set env so we avoid it by deferring to the default env.
*/

import { TFabricEnv } from './types';
import { getEnv as getBrowserEnv } from './browser';

Expand All @@ -12,8 +21,3 @@ export const getEnv = () => env || getBrowserEnv();
export const getDocument = (): Document => getEnv().document;

export const getWindow = (): Window => getEnv().window;

export const setEnvForTests = (window: Window) => {
env.document = window.document;
env.window = window;
};
Copy link
Member

Choose a reason for hiding this comment

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

this was specific for tests, for now setEnv is a necessity of delaying an import / definition issue, and i don't think it is a feature yet. I would like this to stay setEnvForTests ( and even that was some kind of issue of testing and not a real need ) and setEnv to not be exposed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking we should expose setEnv for devs who work on a strange env, someone wrote about it in #8208

Copy link
Member

Choose a reason for hiding this comment

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

in case we will have specific pr with new feature for it after evaluating which are the side effects.

46 changes: 26 additions & 20 deletions src/env/node.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
/* eslint-disable no-restricted-globals */
import type { Canvas as NodeCanvas } from 'canvas';
import { JSDOM } from 'jsdom';
import utils1 from 'jsdom/lib/jsdom/living/generated/utils.js';
import utils2 from 'jsdom/lib/jsdom/utils.js';
import utils from 'jsdom/lib/jsdom/living/generated/utils.js';
import { config } from '../config';
import { NodeGLProbe } from '../filters/GLProbes/NodeGLProbe';
import { setEnv } from './index';
import { TCopyPasteData, TFabricEnv } from './types';

const { implForWrapper: jsdomImplForWrapper } = utils1;
const { Canvas: nodeCanvas } = utils2;
const { implForWrapper: jsdomImplForWrapper } = utils;

const copyPasteData: TCopyPasteData = {};

const { window: virtualWindow } = new JSDOM(
const { window: fabricWindow } = new JSDOM(
ShaMan123 marked this conversation as resolved.
Show resolved Hide resolved
decodeURIComponent(
'%3C!DOCTYPE%20html%3E%3Chtml%3E%3Chead%3E%3C%2Fhead%3E%3Cbody%3E%3C%2Fbody%3E%3C%2Fhtml%3E'
),
Expand All @@ -20,31 +20,37 @@ const { window: virtualWindow } = new JSDOM(
FetchExternalResources: ['img'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is needed, it seems to be a relic of the past

Copy link
Member

Choose a reason for hiding this comment

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

it could still be needed to actually load base64 images.
Since canva is not mandatory for jsdom, loading images is completely optional.
That is my understanding, if you want them to actually be loaded, you have to ask for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is dead.
Not typed and I searched the repo and couldn't find it.

Copy link
Member

Choose a reason for hiding this comment

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

yes is not more there indeed.
It is replaced by resources 'usable' we have below.

},
resources: 'usable',
// needed for `requestAnimationFrame`
pretendToBeVisual: true,
}
);

const fabricDocument = virtualWindow.document;
const fabricWindow = virtualWindow;

const isTouchSupported =
'ontouchstart' in fabricWindow ||
'ontouchstart' in fabricDocument ||
(fabricWindow &&
fabricWindow.navigator &&
fabricWindow.navigator.maxTouchPoints > 0);

config.configure({
devicePixelRatio: fabricWindow.devicePixelRatio || 1,
});

export const getNodeCanvas = (canvasEl: HTMLCanvasElement) => {
const impl = jsdomImplForWrapper(canvasEl);
return (impl._canvas || impl._image) as NodeCanvas;
};

export const getEnv = (): TFabricEnv => {
return {
document: fabricDocument,
document: fabricWindow.document,
window: fabricWindow,
isTouchSupported,
isLikelyNode: true,
nodeCanvas,
jsdomImplForWrapper,
isTouchSupported: false,
GLProbe: new NodeGLProbe(),
dispose(element) {
const impl = jsdomImplForWrapper(element);
if (impl) {
impl._image = null;
impl._canvas = null;
// unsure if necessary
impl._currentSrc = null;
impl._attributes = null;
impl._classList = null;
}
},
copyPasteData,
};
};
Expand Down
7 changes: 3 additions & 4 deletions src/env/types.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { Canvas } from 'canvas';
import { GLProbe } from '../filters/GLProbes/GLProbe';

export type TCopyPasteData = {
copiedText?: string;
Expand All @@ -8,8 +8,7 @@ export type TFabricEnv = {
document: Document;
window: Window;
isTouchSupported: boolean;
isLikelyNode: boolean;
nodeCanvas?: Canvas;
jsdomImplForWrapper?: any;
GLProbe: GLProbe;
dispose(element: Element): void;
copyPasteData: TCopyPasteData;
};
Loading