Skip to content
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

feat: Disallow import of user-designated server-only modules from the client #6422

Merged
merged 35 commits into from Sep 5, 2022

Conversation

tcc-sejohnson
Copy link
Contributor

@tcc-sejohnson tcc-sejohnson commented Aug 30, 2022

Allow users to take advantage of the same dev- and run-time graph analysis that $env/*/private modules benefit from. Any module named *.server.* will be disallowed for client import. This does not include modules inside node_modules.

TODO:

  • Stop re-resolving lib
  • Find a place to put the documentation (+ does it need more? Less? I wrote a draft down in the comments on this issue.)

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2022

🦋 Changeset detected

Latest commit: bfb886c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

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

@tcc-sejohnson
Copy link
Contributor Author

Hah well, I've broken things. Will fix later.

@tcc-sejohnson tcc-sejohnson marked this pull request as draft August 30, 2022 03:23
@Rich-Harris
Copy link
Member

would we want to add a check to make sure the file is in the project directory, eg. "Is not in node_modules but is somewhere inside the project directly and matches the pattern"?

implemented in 2f16ba2

@accuser
Copy link

accuser commented Sep 6, 2022

👍🏼 Really useful addition to Svelte Kit. I wonder if this could be extended to disallow client import of modules from a configured path, e.g., $lib/server/**?

@tcc-sejohnson
Copy link
Contributor Author

👍🏼 Really useful addition to Svelte Kit. I wonder if this could be extended to disallow client import of modules from a configured path, e.g., $lib/server/**?

I think if we were going to do that, it would have to be from one specific location, i.e. $lib/server. Any other (more configurable) approach strikes me as becoming fairly complex fairly quickly.

@accuser
Copy link

accuser commented Sep 6, 2022

I think if we were going to do that, it would have to be from one specific location, i.e. $lib/server. Any other (more configurable) approach strikes me as becoming fairly complex fairly quickly.

Agree. I welcome the utility in the *.server.* naming, but this could become noisy. Having a single location under which I could organise modules that the client would be disallowed from importing would be extremely useful.

I'm actually not sure if $lib/server is the right name, as others might prefer $lib/services or $lib/private.

@dummdidumm
Copy link
Member

If this becomes an option within the files object that could make it configurable while restricting it to not get out of hand.

@tcc-sejohnson
Copy link
Contributor Author

If this becomes an option within the files object that could make it configurable while restricting it to not get out of hand.

From a configuration standpoint we could provide it in config.kit.files, but we'd probably want to assert that it's a subdirectory of config.kit.files.lib. That's technically more opinionated than we have to be (we could allow folders anywhere in the project tree, as long as they're in the project tree), but it seems like the right kind of thing to be opinionated about...

@tcc-sejohnson
Copy link
Contributor Author

@accuser @dummdidumm Implemented in #6623

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants