Skip to content

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

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Conversation

benmccann
Copy link
Member

assuming this is unobjectionable, I will add enforcement of it to the sveltejs eslint config

Copy link

changeset-bot bot commented Sep 11, 2024

⚠️ No Changeset found

Latest commit: 510b89b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Conduitry
Copy link
Member

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?

@benmccann
Copy link
Member Author

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

@dummdidumm
Copy link
Member

yeah, I don't really care about making those work on Deno

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)

@benmccann
Copy link
Member Author

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

@dummdidumm
Copy link
Member

What does this eslint rule do exactly? If this makes it possible to enable it, should it just be added to this PR?

@benmccann
Copy link
Member Author

The eslint rule checks that process is imported

The eslint rules live in another repo: https://github.com/sveltejs/eslint-config

@dummdidumm
Copy link
Member

The eslint rules live in another repo

Are you implying that if this gets merged you add a commit to that repo to then get the updated rules from there?

@benmccann
Copy link
Member Author

Yes

@Rich-Harris
Copy link
Member

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

@Rich-Harris Rich-Harris merged commit 6b69de7 into main Sep 16, 2024
9 checks passed
@Rich-Harris Rich-Harris deleted the import-process branch September 16, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants