-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix: import process for improved Deno compatibility #13204
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
Conversation
|
I have mixed feelings about trying to make internal scripts (that will never be called by users of SvelteKit) work on additional systems/runtimes. This change itself is pretty harmless, but are we in the future going to be making other changes to these files to appease Deno/Bun/others? |
yeah, I don't really care about making those work on Deno. But the easiest way to avoid regressions in the real code is just enable the eslint check for it globally since it's easy to address and not harmful. I'd rather avoid overly complicating the eslint config by specifying which files the check should or should not apply to |
What is "those" referring to? I'm not sure in which way you're agreeing with @Conduitry here, because this is something to appease Deno so to speak (though I'm ok with it given the small change, and I agree about the eslint rule) |
I'm saying that while this change helps Deno I don't think we need to commit to making build scripts work on Deno. I'm just doing it here out of convenience in order to allow easily enabling the eslint rule |
What does this eslint rule do exactly? If this makes it possible to enable it, should it just be added to this PR? |
The eslint rule checks that The eslint rules live in another repo: https://github.com/sveltejs/eslint-config |
Are you implying that if this gets merged you add a commit to that repo to then get the updated rules from there? |
Yes |
Would rather we lived in a world where Deno just accepted the unfortunate reality that it needs to be Node compatible to be relevant, instead of inching towards that realisation in dribs and drabs. But since we don't live in such a world, and since this is an unobtrusive and arguably good change, I say we merge it. Am definitely not committing to caring about running our local dev-time scripts in non-Node-compatible runtimes in future though |
assuming this is unobjectionable, I will add enforcement of it to the sveltejs eslint config