copy dynamic import chunks to functions dirs #174
Conversation
There was a problem hiding this comment.
@lindsaylevine, this looks good as a fix for the issue.
You can discard my comments if you feel they are not relevant
| // Copy page | ||
| const nextPageCopyPath = join(functionDirectory, "nextPage.js"); | ||
| const nextPageCopyPath = join(functionDirectory, "nextPage", "index.js"); |
There was a problem hiding this comment.
The reason behind this change is to organize all the page files in the same directory, or is it also required for the fix to work?
There was a problem hiding this comment.
I believe this is because the chunks are imported relative to a parent dir (e.g. ../[chunk]) 😕
There was a problem hiding this comment.
yeah, what jason said :( the lambda generated by next (that we previous renamed to nextPage.js to pair with next_whatever_the_page_name_is.js) is searching for the chunks one level up at ../ 💀
| const { NEXT_DIST_DIR } = require("../config"); | ||
|
|
||
| // Check if there are dynamic import chunks and copy to the necessary function dir | ||
| const copyDynamicImportChunks = (functionPath) => { |
There was a problem hiding this comment.
Does this mean we're copying all chunks to all functions?
I was also worried users can control the chunks configuration until I found this
There was a problem hiding this comment.
if we're copying all chunks, we might hit size limits for particularly inefficient sites. I think 50MB is the cap, so if people are doing terrible things like including all of lodash/moment/etc. we might see issues opened down the road
There was a problem hiding this comment.
yeah, it does. i think it should be okay for now and can definitely revisit later if issues arise from this implementation. reading the lambdas for the chunk names should be an "if absolutely necessary" approach imo 😂
| const { NEXT_DIST_DIR } = require("../config"); | ||
|
|
||
| // Check if there are dynamic import chunks and copy to the necessary function dir | ||
| const copyDynamicImportChunks = (functionPath) => { |
There was a problem hiding this comment.
if we're copying all chunks, we might hit size limits for particularly inefficient sites. I think 50MB is the cap, so if people are doing terrible things like including all of lodash/moment/etc. we might see issues opened down the road
| // Copy page | ||
| const nextPageCopyPath = join(functionDirectory, "nextPage.js"); | ||
| const nextPageCopyPath = join(functionDirectory, "nextPage", "index.js"); |
There was a problem hiding this comment.
I believe this is because the chunks are imported relative to a parent dir (e.g. ../[chunk]) 😕
| @@ -47,8 +48,11 @@ const setupNetlifyFunctionForPage = ({ | |||
| }); | |||
| }); | |||
|
|
|||
| // Copy any dynamic import chunks | |||
| copyDynamicImportChunks(functionDirectory); | |||
There was a problem hiding this comment.
I love how you contained this so we can drop it easily if/when Next fixes this upstream ❤️
lindsaylevine
force-pushed
the
ll/est-dynamic-imports
branch
from
0aacd11 to
acb37ef
Compare
Fixes #121