-
-
Notifications
You must be signed in to change notification settings - Fork 48
feat: no-not-data-props-in-kit-pages #283
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: no-not-data-props-in-kit-pages #283
Conversation
🦋 Changeset detectedLatest commit: cde30a5 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 |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
cfde211
to
a65362a
Compare
a65362a
to
7a41b55
Compare
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.
Thank you for this PR!
I made some comments.
}, | ||
|
||
// export let xxx | ||
[`ExportNamedDeclaration > VariableDeclaration > VariableDeclarator > Identifier`]: |
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.
I think it's not enough to just check the Identifier
.
export let {a, b} = obj
I think it's better to select a VariableDeclarator
and report an node.id
other than Identifire
, or something other than data
or error
.
"ExportNamedDeclaration > VariableDeclaration > VariableDeclarator"
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.
Oh export let { data, errors } = { data: {}, errors: {} }
is correct way.
https://svelte.dev/repl/61922177a5944a5294e9551787479d6a?version=3.52.0
I updated the logic, so could you please review it again?
type: "problem", | ||
}, | ||
create(context) { | ||
if (!isKitPageComponent(context)) return {} |
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.
I think isKitPageComponent
needs to be changed to be more strict.
I think we need to check if the file name starts with +
.
For filenames that don't start with +
, it may just be a component.
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.
I added check logic and fixed tests.
@@ -0,0 +1,6 @@ | |||
<script> | |||
export let data | |||
export let errors |
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.
I saw this and noticed that errors
is also allowed.
So I think it would be better to change the rule name (no-not-data-props-in-kit-pages
) from what I suggested 😓.
Do you have any good ideas?
For example, how about a name like valid-prop-names-in-kit-pages
?
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.
I changed the rule name according to your suggestion!
I'm sorry, could you change the namespace of settings? #290 |
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.
LGTM! Thank you!
resolve: #241
This is based on #281.
So At first, we should merge #281.