use the new resolve.builtins vite config to specify the cloudflare and node builtins#52
use the new resolve.builtins vite config to specify the cloudflare and node builtins#52dario-piotrowicz wants to merge 2 commits intomainfrom
resolve.builtins vite config to specify the cloudflare and node builtins#52Conversation
dario-piotrowicz
force-pushed
the
dario/bump-vite-to-beta8
branch
from
c0ac005 to
cd6d5fa
Compare
dario-piotrowicz
force-pushed
the
dario/cf-builtins
branch
3 times, most recently
from
400c034 to
0a95cfa
Compare
dario-piotrowicz
changed the title
dario-piotrowicz
force-pushed
the
dario/bump-vite-to-beta8
branch
from
6b077cc to
debe5e0
Compare
dario-piotrowicz
force-pushed
the
dario/cf-builtins
branch
from
0a95cfa to
c627bad
Compare
| '@cloudflare/workers-types': ^4.20241112.0 | ||
| 'typescript': ^5.6.2 | ||
| 'vite': '6.0.0-beta.10' | ||
| 'vite': '/Users/dario/Repos/my-repos/vite/packages/vite' |
There was a problem hiding this comment.
Clearly this PR won't be ready to merge until the version of vite with the resolve.builtins config is (hopefully) released
dario-piotrowicz
changed the title
resolve.builtins vite config to specify the cloudflare and node builtins
dario-piotrowicz
force-pushed
the
dario/cf-builtins
branch
from
c627bad to
add5aa8
Compare
| ? path.resolve(userConfig.root, options.main) | ||
| : path.resolve(options.main), | ||
| external: [...cloudflareBuiltInModules, ...getNodeCompatExternals()], | ||
| external: [...getNodeCompatExternals()], |
There was a problem hiding this comment.
@petebacondarwin we set these node externals unconditionally even if the worker is not under nodejs_compat, is that ok?
Or should we do it conditionally? (similarly to the addNodeBuiltinsIfNeeded function that I'm adding here)
There was a problem hiding this comment.
We can do it conditionally if you like. I was being a bit lazy perhaps?
There was a problem hiding this comment.
I can do it as part of this PR since I'm already adding the other function, so adding this logic should be quite simple 🙂
I just wanted to double check with you first because maybe them being always external for builds was intentional for some reason and/or it doesn't have real/significative downsides
anyways I can do this conditionally here, thanks 🙂
dario-piotrowicz
force-pushed
the
dario/cf-builtins
branch
from
666015b to
8ad4122
Compare
…and node builtins and remove the ad-hoc `cloudflare:` handling logic
dario-piotrowicz
force-pushed
the
dario/cf-builtins
branch
from
8ad4122 to
147342e
Compare
2 participants
resolves #46
paired with this PR I created in the Vite repo: vitejs/vite#18584
Note
The changes in vitejs/vite#18584 are most likely going to make it into vite around Jannuary (discord message) so this PR will be blocked until then