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

@W-17471447 Add a ssr processor and update ssr rule #185

Merged
merged 19 commits into from
Jan 23, 2025

Conversation

abhagta-sfdc
Copy link
Collaborator

@abhagta-sfdc abhagta-sfdc commented Jan 10, 2025

  • The ssr processor here processes the component js files and checks whether the component is SSR capable or not.To check for if a cmp is ssrable or not, the processor looks for particular capabilites define in the js-meta.xml file of component
  • If a cmp is ssrable then new virtual files with content of ssr components js files with same name but .ssrjs extension is created.
  • This enables you to target these new virtual files with .ssrjs extension and apply only ssr rules to these files
  • The expectation is to use the processor to create a virtual file with .ssrjs for all the SSR component js filesr and use these virtual files to apply SSR rules defined here to ssr component js files only.
  • That allows you to apply both ssr rules to ssr specific js(new virtual files with .ssrjs extension) files and apply non ssr rules to both ssr+non-ssr js files

@abhagta-sfdc abhagta-sfdc changed the title Abhagta/processor marker @W-17471447 Add a ssr processor and update ssr rule for marker checks Jan 10, 2025
@abhagta-sfdc abhagta-sfdc changed the title @W-17471447 Add a ssr processor and update ssr rule for marker checks @W-17471447 Add a ssr processor and update ssr rule Jan 10, 2025
@abhagta-sfdc abhagta-sfdc marked this pull request as draft January 10, 2025 11:19
@wjhsf
Copy link
Contributor

wjhsf commented Jan 10, 2025

  • If a cmp is ssrable the js file's content for that component is marked with a /* ESLINT_SSR_TRUE */marker at the top,if not the file is passed for rule validation without any changes.

That's gonna mess with things like eslint-plugin-header and anything else that relies on a top-of-file directive.

@abhagta-sfdc abhagta-sfdc force-pushed the abhagta/processor-marker branch from 26bd4db to b1ab9be Compare January 10, 2025 21:49
@abhagta-sfdc
Copy link
Collaborator Author

abhagta-sfdc commented Jan 10, 2025

  • If a cmp is ssrable the js file's content for that component is marked with a /* ESLINT_SSR_TRUE */marker at the top,if not the file is passed for rule validation without any changes.

That's gonna mess with things like eslint-plugin-header and anything else that relies on a top-of-file directive.

Yeah I noticed that, thats why this I pushed it to draft, I have updated the processor to add a new file with extension .ssrjs for every js file of ssr component which can be used in seperate config to target ssr js files with **/*.ssrjs pattern.
I will test it thouroughly on Monday and move this pr to Ready for Review if everything looks good

@abhagta-sfdc abhagta-sfdc marked this pull request as ready for review January 13, 2025 07:12
@abhagta-sfdc abhagta-sfdc requested review from ravijayaramappa and removed request for ravijayaramappa January 20, 2025 05:25
docs/processors/ssr.md Outdated Show resolved Hide resolved
lib/processors/ssr.js Outdated Show resolved Hide resolved
lib/processors/ssr.js Outdated Show resolved Hide resolved
abhagta-sfdc and others added 4 commits January 21, 2025 07:31
@ravijayaramappa ravijayaramappa requested a review from wjhsf January 21, 2025 15:33
Copy link
Contributor

@wjhsf wjhsf left a comment

Choose a reason for hiding this comment

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

Creating a virtual file that's a clone of an existing file feels weird. It'll tell users that there's an error in a file that doesn't even exist. And you'll end up processing each file twice, just to conditionally apply a subset of rules that you might not even be using.

It feels more intuitive to me to have this configured at the rule level. Users can just treat it like a regular eslint rule, and not have to learn any new quirks. The potential downside is doing the XML check for each file for every rule, but hopefully caching the result will make that a non-issue. Parsing ASTs and file reads are both expensive operations, so I don't actually know which would be more performant. Would be worthwhile to compare implementations.


To make this configurable at the rules level, add this function to the utils:

        function shouldLintSsrRules(context) {
            const userConfig = context.settings && context.settings.lwc && context.settings.lwc.autoDetectSsrCapabilities;
            if (typeof userConfig === 'boolean') {
                return userConfig
            }
            return hasSSRCapabilities(path.dirname(context.filename))
        }

(Note that this suggestion uses shared settings, rather than per-rule options, but it doesn't matter which you use. You could even use both!)

And then just add this short snippet to every SSR rule:

        if (!shouldLintSsrRules(context)) {
            return {};
        }

Comment on lines +49 to +55
bundle && bundle.capabilities
? bundle.capabilities.some((capabilityObj) =>
Array.isArray(capabilityObj.capability)
? capabilityObj.capability.some((cap) => SSR_CAPABILITIES.includes(cap))
: SSR_CAPABILITIES.includes(capabilityObj.capability),
)
: false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bundle && bundle.capabilities
? bundle.capabilities.some((capabilityObj) =>
Array.isArray(capabilityObj.capability)
? capabilityObj.capability.some((cap) => SSR_CAPABILITIES.includes(cap))
: SSR_CAPABILITIES.includes(capabilityObj.capability),
)
: false;
bundle?.capabilities?.some((capabilityObj) =>
Array.isArray(capabilityObj.capability)
? capabilityObj.capability.some((cap) => SSR_CAPABILITIES.includes(cap))
: SSR_CAPABILITIES.includes(capabilityObj.capability),
) ?? false;

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, looks like the eslint enforced on this package is on an old version of ECMAScript, so this won't pass. We should update that at some point... 😅

@abhagta-sfdc
Copy link
Collaborator Author

abhagta-sfdc commented Jan 22, 2025

Creating a virtual file that's a clone of an existing file feels weird. It'll tell users that there's an error in a file that doesn't even exist. And you'll end up processing each file twice, just to conditionally apply a subset of rules that you might not even be using.

Not really from the perspective of user, the eslint output still showcases ssr violations reported under original ssr file (not the virtual one)
Screenshot 2025-01-17 at 11 10 39 PM

And you'll end up processing each file twice, just to conditionally apply a subset of rules that you might not even be using.

Aren't we going to deal with something similar even with rule level approach, you will be processing each file for every rule.
I already proposed the rule level check but it was pushed back by @ravijayaramappa as at rule level we should only deal with the js content and should not be processing other files.Also personally I feel both approaches have their pros and cons but processor one seems to be more efficient.

Also considering this is a public plugin, processor approach makes it easier for 3rd party users to apply any new ssr specific file without dealing with ssr capability checks at rule level

It feels more intuitive to me to have this configured at the rule level. Users can just treat it like a regular eslint rule, and not have to learn any new quirks.

I think its a tradeoff between adding the header check to every ssr rule vs adding a separate config for ssrjs extension.

Also most probably this is a interim solution. @ravijayaramappa can comment more on this but I believe there are plans to seperate ssr components under a different path (maybe having /ssr/ in their path) which would make it easier to target them using files pattern.

@ravijayaramappa
Copy link
Contributor

@wjhsf I agree with Ayush's comment's above. The two drawbacks of including the detection logic in the rule implementation itself is:

  1. The same rules are used in the lwc-platform-public compiler APIs where eslint is invoked on in-memory LWC content. In that case, we do not have access to the file system to read the meta.xml file content. Meaning the detection logic now has to aware of whether the content being linted is from a file system or in-memory.
  2. The pre-processor approach encapsulates the SSR detection logic and offers it as a api from salesforce/eslint-plugin-lwc, allowing users to add new SSR rules without concerning themselves about what component qualifies as a SSR component or not.

@abhagta-sfdc
Copy link
Collaborator Author

abhagta-sfdc commented Jan 22, 2025

@wjhsf @ravijayaramappa lets keep this pr on hold for now. I am trying to test current changes with 2.1.0-beta.1 version in core but have been running into issues while applying processor(its having trouble dealing with virtual files as ts parser needs the file to be listed in tsconfig) in core 1st party configs.

I will spend some more time testing it .Will share an update here.
Thanks!

@wjhsf
Copy link
Contributor

wjhsf commented Jan 22, 2025

Aren't we going to deal with something similar even with rule level approach, you will be processing each file for every rule.

I meant the step parsing the ASTs, rather than reading the files. By using the processor to create file.js and file.ssrjs, there's now two ASTs to parse, rather than one.

The same rules are used in the lwc-platform-public compiler APIs where eslint is invoked on in-memory LWC content. In that case, we do not have access to the file system to read the meta.xml file content. Meaning the detection logic now has to aware of whether the content being linted is from a file system or in-memory.

Does the in-memory content have to conditionally detect SSR capabilities? How is that detection done?


If it doesn't look weird in the console for the end user, and if we're constrained by not always having the file system available, then I'm fine with this approach.

@ravijayaramappa
Copy link
Contributor

Does the in-memory content have to conditionally detect SSR capabilities? How is that detection done?

The Invokers of the compiler, e.g Aura compiler reads the content of the meta.xml file and passes down the signal via a compiler flag.

@abhagta-sfdc abhagta-sfdc merged commit 409927b into salesforce:master Jan 23, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants