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

fix: disambiguate render in module script #2667

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

paoloricciuti
Copy link
Member

One day i'll learn how to properly generate the tests...sorry if i fumbled.

This fixes a problem that you get when you import or create a variable/function named render in the module...since we create a render function ourselves it conflicts with it giving a confusing error to the user.

It also just occured to me that maybe we can even rename the function to something less usual so that it has less chance of conflicting with the user (now that we have a global variable with the name could even be a random identifier so that even if by any change it conflicts with the user it should go away the next restart of language server).

I think this should work fine but I hope the name (or the length of the name) was not referenced somewhere else so please have an extra eye for this things.

@paoloricciuti
Copy link
Member Author

For the moment i've run the tests in v5...not sure how to generate the v4 version

@paoloricciuti
Copy link
Member Author

Oh small note: this technically "over fires"...since it's looking for all the identifiers in the module tag it could rename the render function even for x.render for example...I don't think is worth it to be precise here since it's not a big deal (the user will never see this) and it's not a performance hit and better be over firing than specifying every case and maybe missing something. But i can fix it if we want.

@dummdidumm
Copy link
Member

what about just renaming it to $$render instead? Starts with a disallowed character, so no possibility of clashes. Will mean we gotta adjust basically every test but that can happen with one test-folder-level search&replace I think.

@paoloricciuti
Copy link
Member Author

what about just renaming it to $$render instead? Starts with a disallowed character, so no possibility of clashes. Will mean we gotta adjust basically every test but that can happen with one test-folder-level search&replace I think.

Ohhh i like this...let me do it

@paoloricciuti
Copy link
Member Author

@dummdidumm what do you think of still using a variable name since i've already did the work of replacing it everywhere? this way it could be easier to do if we later want to change it for some reason. Otherwise i can just revert everything and manually change them

@dummdidumm
Copy link
Member

We can keep it, but there's no point in passing it through functions, I would just do export const RENDER = '$$render' and use that

@paoloricciuti
Copy link
Member Author

We can keep it, but there's no point in passing it through functions, I would just do export const RENDER = '$$render' and use that

Uh yeah...i think there's some fixture that needs to be updated for the language server too (and also sourcemaps for svelte 4)...could you guide me on how to rerecord them?

@paoloricciuti
Copy link
Member Author

Oh there was indeed some hardcoded render check in the language server so tests are failing for a good reason. I'll fix later.

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.

2 participants