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

fix: fetch assets in dynamic content (#1369, #1587, #1630, #1936, #2064) #2474

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mskec
Copy link
Contributor

@mskec mskec commented Jan 5, 2024

Reopening PR #2240

This PR fixes crashes with images inside dynamic content.

From looking at it, the problem was that assets weren't loaded for images inside dynamic content because resolveAssets was executed before resolvePagination.
To resolve this, I've added resolveAssets as a step in relayout which forced me to add a bunch of async/await all over resolvePagination.

fixes: #1369, #1587, #1630, #1936, #2064

Copy link

changeset-bot bot commented Jan 5, 2024

⚠️ No Changeset found

Latest commit: 8c32948

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@takethefake
Copy link

thanks for the contribution @mskec - looks promising! What needs to be done so this could be released @diegomura ? We also experienced this issue and are working with a nasty workaround. This fix would help us a lot!

@jkergal
Copy link

jkergal commented Feb 26, 2024

thanks for the contribution @mskec - looks promising! What needs to be done so this could be released @diegomura ? We also experienced this issue and are working with a nasty workaround. This fix would help us a lot!

Hey, can you share your nasty workaround? Would be great! :)

@takethefake
Copy link

hej @jkergal I required to have a different header on page 1 vs page 2 and following. Instead of putting all content inside only one page element and leaving the page wraps to the page element I've added another page element.

Therefore I need to separate my content between both page elements to apply different headers.

@mskec
Copy link
Contributor Author

mskec commented Feb 26, 2024

You could try to use patch-package and apply the fix.
I see there's some conflict now. I will try to resolve it and update the PR.

@diegomura can you please give us some comment on this. Do you see a better way to fix this bug?

@takethefake
Copy link

takethefake commented Feb 26, 2024

hey @mskec maybe add a changeset and rebase to its main to make it easier for @diegomura to add this change.

@mskec mskec force-pushed the issue-2064/image-inside-dynamic-content branch 3 times, most recently from ffb096b to 2bd2398 Compare February 26, 2024 13:17
@mskec mskec force-pushed the issue-2064/image-inside-dynamic-content branch from 2bd2398 to fadf37f Compare February 26, 2024 13:18
@mskec
Copy link
Contributor Author

mskec commented Feb 26, 2024

After rebasing with the latest master changes, I'm seeing errors in resolvePagination. I will need more time to check why is it failing.

Error: Worker terminated due to reaching memory limit: JS heap out of memory
 ❯ new NodeError node:internal/errors:405:5
 ❯ [kOnExit] node:internal/worker:287:26
 ❯ Worker.<computed>.onexit node:internal/worker:209:20

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { code: 'ERR_WORKER_OUT_OF_MEMORY' }

@deanwyns
Copy link

deanwyns commented Mar 5, 2024

It seems that you're missing an 'await' in the paginate function (line 263) causing it to infinitely loop until it runs out of memory:

@mskec
Copy link
Contributor Author

mskec commented Mar 5, 2024

Good catch @deanwyns, thank you :)

@ishak14
Copy link

ishak14 commented Mar 6, 2024

also waiting for this fixed. just updated to 7.7.1 and noticed that my PDF was not working. problem occurs when loading images dynamically

@mskec
Copy link
Contributor Author

mskec commented Mar 28, 2024

Hi @diegomura, can you give us some comment on this fix? Do you have concerns about the way this is fixed?

I created the same PR #2240 in March 2023 and never received any feedback from you.

@hect1c
Copy link

hect1c commented Mar 28, 2024

I tried to patch the changes in my file but after doing so I get the following error

error TypeError: Cannot read properties of undefined (reading 'width')
    at resolvePageStyles (index.js:1328:40)
    at Array.map (<anonymous>)
    at resolveStyles (index.js:1354:32)
    at _callee$ (index.js:89:23)
    at tryCatch (regeneratorRuntime.js:50:16)
    at Generator.eval (regeneratorRuntime.js:138:17)
    at Generator.eval [as next] (regeneratorRuntime.js:79:21)
    at asyncGeneratorStep (asyncToGenerator.js:7:24)
    at _next (asyncToGenerator.js:26:9)

Did anyone get the same issue?

@jalalmanafi
Copy link

I have same issue, any solution?

@jalalmanafi
Copy link

jalalmanafi commented Mar 30, 2024

I tried to patch the changes in my file but after doing so I get the following error

error TypeError: Cannot read properties of undefined (reading 'width')
    at resolvePageStyles (index.js:1328:40)
    at Array.map (<anonymous>)
    at resolveStyles (index.js:1354:32)
    at _callee$ (index.js:89:23)
    at tryCatch (regeneratorRuntime.js:50:16)
    at Generator.eval (regeneratorRuntime.js:138:17)
    at Generator.eval [as next] (regeneratorRuntime.js:79:21)
    at asyncGeneratorStep (asyncToGenerator.js:7:24)
    at _next (asyncToGenerator.js:26:9)

Did anyone get the same issue?

I found issue on my code, check your content if it comes from API you have to check field you use in PDF file

{data?.field ? <Text>{data?.field}</Text> : null }

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

Successfully merging this pull request may close these issues.

Render prop method on View does not work for Images
7 participants