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

Remove node-canvas from @jbrowse/core dependencies #3044

Merged
merged 2 commits into from
Jun 21, 2022
Merged

Conversation

cmdcolin
Copy link
Collaborator

If we detect isNode is true, then we use global variables nodeCreateCanvas and nodeImage to perform canvas operations. These are global variables that apps that know that they run under node can set (namely, @jbrowse/img which runs under node and our integration tests which perform canvas snapshots)

We do not try to use the global variables until client code calls new Image or createCanvas, which allows us an opportunity to set nodeCreateCanvas and nodeImage after initialization e.g. doesn't depend on the order that dependencies are included (if we tried to statically detect whether e.g. nodeCreateCanvas exists then it would depend on whatever sets global.nodeCreateCanvas to be included first for example, and it is tricky to guarantee order of initialization)

Would maybe help with issues such as #1739

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Jun 19, 2022
return new Promise((resolve, reject) => {
img.onload = () => resolve(img)
img.onerror = reject
img.src = dataUri
})
}
ImageBitmapType = Image
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ImageBitmapType now not available statically (due to trying not to depend on initialization order). There could be alternative ways e.g. we make the ponyfill return a callback that gives us the createCanvas callback perhaps, but this is a bit intrusive.

@cmdcolin cmdcolin added enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Jun 19, 2022
@cmdcolin cmdcolin force-pushed the canvas_out_of_core branch from 62a4049 to 70e4d05 Compare June 19, 2022 16:59
@cmdcolin cmdcolin force-pushed the canvas_out_of_core branch from 70e4d05 to ffa1f7b Compare June 19, 2022 17:38
@cmdcolin cmdcolin marked this pull request as ready for review June 19, 2022 17:44
@codecov
Copy link

codecov bot commented Jun 19, 2022

Codecov Report

Merging #3044 (ffa1f7b) into main (39af675) will decrease coverage by 0.01%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #3044      +/-   ##
==========================================
- Coverage   61.71%   61.69%   -0.02%     
==========================================
  Files         591      591              
  Lines       26919    26922       +3     
  Branches     6527     6526       -1     
==========================================
- Hits        16612    16610       -2     
- Misses      10004    10009       +5     
  Partials      303      303              
Impacted Files Coverage Δ
products/jbrowse-img/src/index.js 0.00% <0.00%> (ø)
products/jbrowse-img/src/index.testmod.js 0.00% <0.00%> (ø)
packages/core/util/offscreenCanvasPonyfill.tsx 51.51% <100.00%> (+1.51%) ⬆️
products/jbrowse-web/src/tests/util.js 92.72% <100.00%> (+0.27%) ⬆️
packages/core/util/analytics.ts 89.79% <0.00%> (-2.05%) ⬇️
.../linear-genome-view/src/LinearGenomeView/index.tsx 84.61% <0.00%> (-0.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39af675...ffa1f7b. Read the comment docs.

@cmdcolin
Copy link
Collaborator Author

I think it should be good to go. The only concern I see is we could more or less use a plain old polyfill for createCanvas instead of this ponyfill but that can probably be changed later if needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant