replace regex replacemend logic in iifefyFunctionFile and handle _ENTRIES declarations
#846
Conversation
|
dario-piotrowicz
force-pushed
the
avoid-iifefy-regex-replace
branch
from
1f679c4
to
273b71b
Compare
|
🧪 Prereleases are available for testing 🧪 @cloudflare/next-on-pagesYou can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/10223327767/npm-package-next-on-pages-846@cloudflare/eslint-plugin-next-on-pagesYou can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/10223327767/npm-package-eslint-plugin-next-on-pages-846 |
…ENTRIES` declarations in `iifefyFunctionFile` replace the regex replacement code that was present to prepare the file contents to be embedded in the iife with more proper AST code modding as part of this change make also sure that `_ENTRIES` identifiers are not modified if they were actually declared inside the function file (as it happens in older Vercel CLI releases)
dario-piotrowicz
force-pushed
the
avoid-iifefy-regex-replace
branch
from
273b71b
to
895cd66
Compare
| * @param fileContents the original file contents | ||
| * @returns the file contents updated and ready to be embedded into an iife | ||
| */ | ||
| function prepareFunctionFileContentForIifeConversion(fileContents: string) { |
There was a problem hiding this comment.
i would be curious to know what the impact on build-time is with this change - are you able to get some numbers for that?
There was a problem hiding this comment.
also, we already use acorn for parsing asts - would that help here as well?
There was a problem hiding this comment.
also, we already use acorn for parsing asts - would that help here as well?
I don't think so since there all we care about is getting (and analyzing) the AST while here we want to actually traverse and manipulate it
There was a problem hiding this comment.
i would be curious to know what the impact on build-time is with this change - are you able to get some numbers for that?
I should be able to get some before vs after numbers if you want 🤔
In any case this runs only on function files (not all js files) so... I hope it's not to costly 😓
Even if this was slower I think that a slower but more correct and robust solution would be preferable than a quick but hacky and brittle one, right?
There was a problem hiding this comment.
i would be curious to know what the impact on build-time is with this change - are you able to get some numbers for that?
Oh wow! it's actually pretty bad! 😖 (thanks so much for the useful comment! I didn't imagine the impact could be this big! ❤️)
I've created a brand new Next.js app with C3 and run the build script, with the latest next-on-pages release it takes around 0.27s but with this PR's prerelease it takes around 2.95s...
I wanted to have a look at a more complex app so I gave the app-playground-edge app a try as well, in that case the build time goes from around 1.75s to around 4.35s... so a bit better but still pretty bad 😕
So maybe it might be best to stick with regex replacement for now... 😓 😖
@vicb what do you think?
There was a problem hiding this comment.
I thought that might happen 😅. It was a big problem with how we originally did things before the big CLI rewrite. Back then, I changed it from two passes over the AST to one pass which helped speed things up quite a bit. So yeah... Umm... It'll slow it down a lot.
I wonder if it might be possible to try and do some of this during the original parsing of the AST? I'm not sure how much would need changing to make it work, but it'd be an interesting idea to look into
There was a problem hiding this comment.
I thought that might happen 😅. It was a big problem with how we originally did things before the big CLI rewrite. Back then, I changed it from two passes over the AST to one pass which helped speed things up quite a bit. So yeah... Umm... It'll slow it down a lot.
I see... 😓 I totally forgot that we had two AST passes 😅 (but yeah I do remember that the refactoring sped up things quite a bit)
I wonder if it might be possible to try and do some of this during the original parsing of the AST? I'm not sure how much would need changing to make it work, but it'd be an interesting idea to look into
That's an interesting idea... maybe? 😕 the issue is that again, there we only analyze the AST and don't have means for actually updating it... so if we were to do something there I think it would likely still be a regex replacement (maybe after we've identified relevant code snippets?), so it feels like no matter what we either take the perf hit or do rely on regex replacements 😕
|
I see no reason to switch to AST for now as your experiment shows it's much slower. But it sounds like the Thanks! |
|
Closing in favour of #852 |
This PR replaces the regex replacements introduced in the
iifefyFunctionFilefunction in #836 with with more proper AST code moddingAs part of this I also noticed that we do not want to update
_ENTRIESidentifiers if they are actually declared in the file (as it happens in older Vercel CLI releases) so I've addressed this too.Note
This PR does not include a changeset since the changes in #836 have not yet been released and this PR simply improves that solution