Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Run check on all addons test#569

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
brettearle wants to merge3 commits intosveltejs:main
base:main
Choose a base branch
Loading
frombrettearle:runCheckOnAllAddonsTest

Conversation

brettearle
Copy link
Contributor

@brettearlebrettearle commentedMay 19, 2025
edited
Loading

closes:#344

blocked by upstream pr on story book

Edit:storybookjs/storybook#31451

@changeset-botchangeset-bot
Copy link

changeset-botbot commentedMay 19, 2025
edited
Loading

⚠️ No Changeset found

Latest commit:a1d8bdb

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

@pkg-pr-newpkg.pr.new
Copy link

pkg-pr-newbot commentedMay 19, 2025
edited
Loading

Open in StackBlitz

npx https://pkg.pr.new/sveltejs/cli/sv@569
npx https://pkg.pr.new/sveltejs/cli/svelte-migrate@569

commit:a1d8bdb

@brettearlebrettearle marked this pull request as draftMay 19, 2025 14:43
@brettearle
Copy link
ContributorAuthor

brettearle commentedMay 19, 2025
edited
Loading

@manuel3108 this is the prototype for making check pass in all addons

Edit: Wont need the tooling comment change

Edit: The paraglide and Lucia hook are different. Would need to update paraglide to magic string implementation to match them. Running parseScript removes jsdoc comments from previous run. In this case paraglide comments were getting overridden when Lucia hook.server ran with ast.

Copy link
Member

@manuel3108manuel3108 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for the PR btw, I will need to properly check everything once I have more time. But looks good and understandable overall.

The only thing i did not find on my inital pass through is to actually runpnpm check on the all-addons test. But maybe I missing something


sv.file(`src/hooks.server.${ext}`, (content) => {
const { ast, generateCode } = parseScript(content);
js.imports.addNamespace(ast, '$lib/server/auth.js', 'auth');
js.kit.addHooksHandle(ast, typescript, 'handleAuth', getAuthHandleContent());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can you please elaborate why we are not using this anymore? What was the problem with this call? Is it potentially fixable insideaddHooksHandle or do we potentially have to check all other callees of this function for similar issues?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Flow issue
-> paraglide adds doc comment to solve checking issues with handler types
-> adds to file correctly
-> Lucia parses script (*main issue here = this removes added doc comments as acorn doesn't support them, from my digging **note I've never used acorn)
-> Lucia adds it's doc comment but resulting file misses paraglide.

I went with the string route as the solution here as it also ties into Ben wanting to add more string returns in issue#557.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm probably missing something, but I don't see any code adding a comment insideaddHooksHandle that could cause this. Also tried running the adapter with JsDoc

Copy link
ContributorAuthor

@brettearlebrettearleJun 8, 2025
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't think it's adding of the comment that fails. I think it is what it holds in memory as the ast (once again it's my first day with acorn and asts). I think it holds everything except the comments. Everytime I tried it with both using ast implementation it would drop 1 of the doc comments

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I will re look into it, I didn't document the why. From memory acorn doesn't read comments back in when it reads the same file back in. So first comment gets dropped when new hook is added, which is why second order has to be strong. But will need to reconfirm my human memory

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

No problem. Acorn/sv is doing some weirdness around comments, but i did not experience anything like this. Maybe it's justesrap thats not printing them correctly or something.

Also please let me know, once you get back to it, about which comment you are talking exactly, as I didn't manage to figure that out.

@brettearle
Copy link
ContributorAuthor

brettearle commentedMay 23, 2025
edited
Loading

Morning@manuel3108
I haven't added the check, the draft was purely to get the maintainers eyes on it make sure I'm sticking to the vision of the code base here since I'm fundamentally changing the Lucia adds hooks implementation from ast to strings.
If I get the nod and go ahead once we work out the creases I'll go through and move paraglide to string style and implement running 'pnpm check' in all add-ons test.

Note: there hasn't been movement on storybook fix upstream yet.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@manuel3108manuel3108manuel3108 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

runcheck command onall-addon test
2 participants
@brettearle@manuel3108

[8]ページ先頭

©2009-2025 Movatter.jp