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(eslint): rule for sync$() #5638

Closed
wants to merge 2 commits into from
Closed

Conversation

Lemon2311
Copy link

@gioboa send me as an example merge #5455, but since then @mhevery in #5572 refactored the code as all the ruleTesters were just placed inside qwik.unit.ts. I made this changes, implemented the noSyncClosure rule, but I can`t seem to find where he moved the ruleTester implemetations so that I can include my test for the noSyncClosure rule.

Copy link

netlify bot commented Dec 26, 2023

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit e4a57fc

@wmertens wmertens changed the title draft for fixing issue#5596 feat(eslint): rule for sync$() Mar 4, 2024
]);

return context.getScope().through.some(scopeVar =>
scopeVar.identifier.name === variable && browserAPIs.has(variable)
Copy link
Member

Choose a reason for hiding this comment

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

Won't this run into trouble when you do { const document = "foo"; sync$(() => document)}?

Since all these are global and globals aren added to the scope array (see the bundles here), would it instead be possible to just check if it's using only globals?

You could even check if they're browser globals but since it would crash immediately for undefined globals I think that's not necessary.

@gioboa
Copy link
Member

gioboa commented May 27, 2024

Hi @Lemon2311 are you still working on this or is it abandoned?

@Lemon2311
Copy link
Author

Hello @gioboa, I haven`t looked into this more since this draft. I saw u assigned @ahmaddonishyar to the issue so I suppose not for now, if he doesn`t fix it I can give it another go then.

Copy link

netlify bot commented Jun 14, 2024

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit c0bef5e

@gioboa
Copy link
Member

gioboa commented Jul 10, 2024

@Lemon2311do you need an help to finalise this PR?

@Lemon2311
Copy link
Author

@gioboa if you want you can continue from where I left off, if not i can look into it in a few days.

@gioboa
Copy link
Member

gioboa commented Jul 10, 2024

Unfortunately I don't have the possibility to look at it in the next days, I'm so sorry

@Lemon2311 Lemon2311 closed this Oct 14, 2024
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.

3 participants