-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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(adapter-node): Enforce dir
to be an actual directory.
#10306
Conversation
🦋 Changeset detectedLatest commit: a74475a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dir
to be an actual directory.dir
to be an actual directory.
I'm unclear on what this is changing, exactly. kit/packages/adapter-node/index.js Lines 85 to 89 in 6b99851
When does this end up pointing to the wrong path? How is webpack involved? Do you have a minimal reproduction of the situation this is trying to address? I don't want to just blindly make the change in this PR, even the |
I mentioned Webpack because I believe it's resolution of I will get to work on pushing a public version of the repo where I first encountered this issue, and look into the possibility of moving the solution to the aforementioned rollup call. Thank you for taking the time to review this PR. |
This is interesting, and helpful, thank you. It does make me think that my proposed place to handle this might not be a good place to handle it. The generated application, basically, expects to be able to trust I'm not sure what to do about this situation. If the bundler can't be configured to provide the correct Is there a reason the whole application needs to be bundled again rather than just having its entrypoint imported as-is by the consuming application? This, to me, is sounding more like a documentation issue, where we stress the importance of the generated applications (for all adapters, but it probably matters most for the Node one) being able to trust the |
Unfortunately, there is: Runtimes provided by certain local cloud companies, or even Electron, do not support features used by the handler, such as Top-Level Awaits or even ES Modules, therefore needing the re-transpilation process.
That is completely fair, and I am pretty saddened by the fact many bundlers do not implement this correctly. However, from my testing (which included deploying to multiple environments), the addition of this one extra check (which is ran during the handler's import, adding little to no overhead) seems to enable the handler to be used in all of those environments, including Electron, and our micro services solution (which runs on multiple local cloud providers, many of which ship outdated node versions).
You are 100% correct - it shouldn't be the application's job to try to fix the issues caused by the bundler, at least in theory. However, in this specific case, this one line fix does not only fix issues caused by bundlers, but also fixes a compatibility with different Node versions, eliminating problems such as the one exemplified by the StackOverflow link provided in my initial comment. I completely agree with you: We are in 2023, these kinds of things shouldn't be necessary, but unfortunately they still are. This change should not introduce any negative effects, and widen the compatibility of |
But why would you use |
The existing adapters are great, however, there are (and probably there are always going to be) projects that require greater flexibility, which is exactly what This PR does not add the custom handler feature - it already exists in SvelteKit (as documented here) - all it does is merely fix a bug caused by some bundlers, thus allowing for wider compatibility for an existing feature. It is also important to remember that while all major cloud companies have adapters, the need to work with smaller, local cloud providers (which provide their own deployment system) may also arise - sometimes, due to regulations/data governance issues; sometimes, simply because of pricing/business issues. |
Seems to have been fixed by #10314 - will test again to confirm. |
Ok, I'm going to go ahead and close this then |
This PR addresses a small issue that tends to happen when trying to utilise the handler generated by
adapter-node
in monorepos that require a bundler like Webpack. For some reason, the current implementation tends to setdir
to a JS file instead of a directory, like/packages/stellar-desktop/dist/b1229d39b38c826bd9a3.js
, resulting in public asset lookups in paths that/packages/stellar-desktop/dist/b1229d39b38c826bd9a3.js/client
(which are clearly invalid).This PR contains a single one-line change that, if verified that
dir
is ajs
file, passes its value throughpath.dirname
to get its actual path. This fixes the code when passed through bundlers, while retaining full functionality with no changes required for either the standalone server or standard usage of the handler. All tests have passed, although I believe including an extra one (as described by checklist item 3) is out of scope for this repo.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Not quite an issue, but there is this StackOverflow Question that recommends an external static asset server configuration as a solution (which goes against SvelteKit's Docs)
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.