-
-
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
feat: add config option to prevent import into client code #12480
base: main
Are you sure you want to change the base?
Conversation
…ing imported to client site code
|
Ive dropped the changeset so far to be added by maintainers. |
After reconsidering the naming, I would suggest naming the attribute serverOnlyPaths |
fix import paths removed unused imports
|
||
const svelte_config = await load_config(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loading the config again could be a breaking change in case a config contains one time setup code that is then executed twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah mentioned it above, didnt saw another way to get the config on this part of code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would have to be found then. using top-level-await here is also not great.
if (configRule.test(id)) return true; | ||
} else if (typeof configRule === 'function') { | ||
const check = configRule(id); | ||
if (typeof check === 'boolean') return check; //always return the boolean to allow exceptions, if its undefined continue to allow multiple rules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
early return should only happen on true, otherwise a return false could give access to an id that would be denied by a later check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would be a design decision. If I want to make a exception and allow a file import, no matter what other rules say, the early return is needed. but depends on execution order then. If a rule dont care, it returns undefined/nothing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would allow for conflicting rules and a "first one wins" scenario, more complex, even harder to understand and not currently documented in the jsdoc in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if really reducing to only one rule and not allowing exceptions i'd drop anything but regex, should be flexible enough IMO
3 different ways to define user-supplied rules seems a bit much, esp. if there is ()=>boolean already, users can just use regex or string compare themselves. But before continuing on this PR please see my comment in #12477 |
the idea was to make the function method for more complicated rules, while string is prefered. ofc function only does work out |
Closes #12477
Added
serverProtectedPathsserverOnlyPaths to add additional rules to prevent import into client codePlease don't delete this checklist! Before submitting the PR, please make sure you do the following:
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:
.Edits