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

ci(): Node build #8632

Merged
merged 62 commits into from
Feb 3, 2023
Merged

ci(): Node build #8632

merged 62 commits into from
Feb 3, 2023

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Jan 31, 2023

Motivation

#8208 + supporting SSR out of the bbox

Description

continues #8622 with additional config changes and fixes to support SSR and remove require calls and awkwardness from fabric env setup.

After this is merged we can change to node index file.

Changes

view the diff from #8622 v6-package-testing...node-build

  • split env file to browser, node and to an index file that is consumed by fabric
  • node entry point - didn't remove imports such as Canvas, Textbox. That can be done in a follow up
  • AnimationFrameProvider - lazy init for SSR, not calling getEnv during import
  • StaticCanvas - moved node methods to node index, fix cloneWithoutData return value to match node StaticCanvas (or any subclass)
  • object default values: workaround for SSR, can be done differently. I want to rename isLikelyNode because we are now certain it is node. Haven't done it yet so the PR isn't polluted. Give me a green light and I will (or in a follow up)
  • created index files for exports, put them under the folders and renamed to index. If you don't want the name to be index to avoid importing from it (though I don't really understand why) revert 8829519 and naming will be restored. reverted e4fab60

Gist

In order to support SSR getEnv can't be called when importing fabric because then env is setup incorrectly because at the time of importing window/document are undefined and only when canvas is initialized then env should be setup.

In Action

Test this config by doing the following:
checkout v6-sandbox
run npm run sandbox start -- -t next
You will see in the browser tab 2 canvases, one with next SSR dynamic import and one plain and simple, both working.
In the node tab hit the download buttons to see the node build in action

OR run in gitpod
Gitpod Ready-to-Code

@asturur
Copy link
Member

asturur commented Feb 2, 2023

let me know when you are done so i can play with the exports and examples.

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

I renamed the output files to index and index.node

This PR ןד ready apart from TS not recognizing
import * as fabric from 'fabric/node'

I tried many things and read a lot of stuff online and wasted 3-4 hours on it so I am giving up
Seems that it is another TS limitation
Tried declaring modules in d.ts file with no success...
Added paths in tsconfig after reading about it and seeing rxjs that has that config and imports work for it but couldn't get it to work for fabric (However I might have managed to get it working but symlinking is known מםא to work is this cases and I didn't test it w/o symlinks)

Let's move on and get back to it.

You can play in v6-sandbox branch which is up to date with this PR

@ShaMan123
Copy link
Contributor Author

We need to consider removing index.ts so vscode and TS don't import that for types. It isn't needed anyways. It is simply an alias for fabric.ts

@ShaMan123
Copy link
Contributor Author

A workaround for type recognition of fabric/node is renaming index.node to node
I think I will do that

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Feb 3, 2023

A workaround for type recognition of fabric/node is renaming index.node to node I think I will do that

Well I don't know. I am leaving as is since it is clearly a TS bug.
The dev can configure paths in tsconfig as follows to fix it:

paths: {
   'fabric/node': './node_modules/fabric/dist/index.node'
}

@asturur
Copy link
Member

asturur commented Feb 3, 2023

looking in a couple of things for node, but in general this PR is good as it is.
I probably don't want to build node with rollup nor having a mjs and cjs version.
I think bundling is a browser issue only but i will open a follow up pr with that

@ShaMan123
Copy link
Contributor Author

Copy link
Member

@asturur asturur left a comment

Choose a reason for hiding this comment

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

i that is the only thing we need to change and we are good to go

@@ -76,6 +76,7 @@
"@rollup/plugin-terser": "^0.3.0",
"@rollup/plugin-typescript": "^11.0.0",
"@types/fs-extra": "^9.0.13",
"@types/jsdom": "^20.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

either use jsdom types for 19 or upgrade jsdom to 20

env = value;
};

export const getEnv = () => env || getBrowserEnv();
Copy link
Member

Choose a reason for hiding this comment

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

i m lost a bit on this thing here.
Why again do we need an index file and the setEnv part if we could use the same trick with isInitialized

Seems like we want fabric importing from here, but having the import statement over the env/node file create a value with setEnv that nullify the browser env. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why again do we need an index file and the setEnv part if we could use the same trick with isInitialized

I am not sure what you are suggesting

Seems like we want fabric importing from here, but having the import statement over the env/node file create a value with setEnv that nullify the browser env. Is that correct?

Yes, correct.

I will some comments

@asturur
Copy link
Member

asturur commented Feb 3, 2023

This PR ןד ready apart from TS not recognizing

i can get the node types recognized if i used them from their folder right away.

@ShaMan123
Copy link
Contributor Author

b326dd6 fixes fabric/node types

@asturur
Copy link
Member

asturur commented Feb 3, 2023

regarding isLikelyNode, i can. count only 4 usage in the codebase.

Webglprobe, objectCaching, parseSVG, and cleanUpJsdomNode

Now my opinion is that cleanUpJsdomNode and webglProbe and objectCaching are env dependendant, and if we don't like the boolean, getEnv could return:

  • EnvCleanUp ( that is a noop in the brower env )
  • shouldCacheByDefault a boolean
  • webglProbe would be a getEnv function that is different for browser and node.

parseSVG should be tested to see if this bug is still actual or isn't anymore.

Do you have a totally different idea?

We can also just rename the boolean, removing the isLikely part, but the bug in parseSVG could be checked anyway if is still actual

@asturur asturur merged commit ec8f561 into master Feb 3, 2023
@ShaMan123
Copy link
Contributor Author

I wanted to add some comments. I will in the follow up.

I agree with what you suggest.
WebGL is browser only. So it shouldn't even exist in the node entry point.
We can mark files with a .browser.ts or .node.ts (shared files stay as is) extension and start having different imports.

Env cleanup is just a jsdom thing. I vote to remove jsdom and extend the node env to support what we really need.
But if you disagree moving the cleanup to env is great.
Even more, we can look into what I did with Canvas, moving the dispose/destroy methods of Canvas/Image to node subclasses. I looked into it in this PR and decided not to go into it for scope reasons, but is doable and pretty simple.

Renaming the boolean is good also, but I prefer removing it. For tests we can add a global flag in the config files.

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