@W-15205324 New rule - disallow import of scoped modules #164
Conversation
| '@salesforce/userPermission', | ||
| '@salesforce/customPermission', |
There was a problem hiding this comment.
@salesforce/userPermission/* and @salesforce/customPermission/* have a third segment in the specifier.
There was a problem hiding this comment.
Correct, for the user-scoped modules I check if an import starts with a value from Set. So, I might not need the 3rd segment. Let me know if you think otherwise.
| const { docUrl } = require('../util/doc-url'); | ||
|
|
||
| const formFactorModule = '@salesforce/client/formFactor'; | ||
| const featureFlagModule = '@salesforce/featureFlag'; |
There was a problem hiding this comment.
The feature flag module needs a flag name as the third segment in the specifier: @salesforce/featureFlag/{flagName}. Similar to the @salesforce/label/{labelReference} scoped module
There was a problem hiding this comment.
From the rule implementation perspective @salesforce/featureFlag is no different than the rest of the user-specific scoped modules
There was a problem hiding this comment.
@lpomerleau , same here. I have a conditional block where I check if an import starts with featureFlagModule. Let me know if you have any concerns regrading that.
| noDynamicUserScopedImport: | ||
| 'Dynamic import of user-scoped modules is not allowed in server-side rendered components. User-scoped values should either be retrieved from a specific cookie or derived from the existing SID cookie.', |
There was a problem hiding this comment.
@khangsfdc What is the recommendation here? A warning like this?
| noDynamicUserScopedImport: | |
| 'Dynamic import of user-scoped modules is not allowed in server-side rendered components. User-scoped values should either be retrieved from a specific cookie or derived from the existing SID cookie.', | |
| noDynamicUserScopedImport: | |
| 'Dynamic import of user-scoped modules is discouraged in server-side components.', |
There was a problem hiding this comment.
I think the dynamic import warning should not be specific to scoped modules. The warning for dynamic imports should just warn the customer that it does not affect SSR output.
There was a problem hiding this comment.
Ok, so there should be a warning for all dynamic imports during SSR. Something like this:
Dynamic imports are not processed on the server and do not affect server-side rendered output.
| } | ||
| ``` | ||
|
|
||
| Example of **correct** code: |
There was a problem hiding this comment.
@khangsfdc What is the recommendation for replacing user-scoped modules if we warn against using dynamic imports?
There was a problem hiding this comment.
This is tough since data providers are the only alternative, but that is not directly consumable by end customers atm.
However, the warning for dynamic imports should just warn the customer that it does not affect SSR output. As long as they are aware of the CSR limitations, it may still be fine to suggest dynamic import here.
cc @kevinv11n
There was a problem hiding this comment.
@khangsfdc it is consumable by end customers through {!User.x} bindling expressions but the user DP does not preload its data currently.
There was a problem hiding this comment.
@nrobertdehault do they have to do anything to enable the binding expressions or is that available to every component within experience sites?
There was a problem hiding this comment.
How about this guidance:
The recommended declarative solution is to use a [data provider](LINK TO PUBLIC DOCS) to fetch this information. The dynamic import approach below is an anti-pattern and should be avoided.
| } | ||
| ``` | ||
|
|
||
| Example of **correct** code: |
There was a problem hiding this comment.
@salesforce/featureFlag/* is not supported, so there isn't any code to put here.
There was a problem hiding this comment.
it is supported internally, can it part of the rule implementation but not documented?
There was a problem hiding this comment.
I didn't think it was supported on the server 🤔 Are the values frozen?
README.md
Outdated
| @@ -86,6 +86,7 @@ To choose from three configuration settings, install the [`eslint-config-lwc`](h | |||
| | [lwc/no-unsupported-ssr-properties](./docs/rules/no-unsupported-ssr-properties.md) | disallow access of unsupported properties in SSR | | | |||
| | [lwc/no-node-env-in-ssr](./docs/rules/no-node-env-in-ssr.md) | disallow usage of process.env.NODE_ENV in SSR | | | |||
| | [lwc/valid-graphql-wire-adapter-callback-parameters](./docs/rules/valid-graphql-wire-adapter-callback-parameters.md) | ensure graphql wire adapters are using 'errors' instead of 'error' | | | |||
| | [lwc/no-import-of-scoped-modules-during-ssr](./docs/rules/no-import-of-scoped-modules-during-ssr.md) | disallow imports of scoped modules in SSR | | | |||
There was a problem hiding this comment.
| | [lwc/no-import-of-scoped-modules-during-ssr](./docs/rules/no-import-of-scoped-modules-during-ssr.md) | disallow imports of scoped modules in SSR | | | |
| | [lwc/no-static-import-of-user-specific-scoped-modules-in-ssrable-components](./docs/rules/no-import-of-scoped-modules-during-ssr.md) | disallow static imports of user-specific scoped modules in SSR | | |
There was a problem hiding this comment.
@nrobertdehault , the format '*-during-ssr' is for the existing rules. Just want to let you know that with the given suggestion, the name convention will go away.
| @@ -0,0 +1,72 @@ | |||
| # Disallow static import of scoped modules during SSR (`lwc/no-import-of-scoped-modules-during-ssr`) | |||
|
|
|||
| During server-side rendering (SSR), these modules resolve to static values on the server and are updated to actual values on the client. | |||
There was a problem hiding this comment.
This is confusing, I would say something like the following:
Static imports of user-specific scoped modules like
@salesforce/user/*are not supported in LWC components marked withlightning__ServerRenderableorlightning__ServerRenderableWithHydration.
| } | ||
| ``` | ||
|
|
||
| Example of **correct** code: |
There was a problem hiding this comment.
How about this guidance:
The recommended declarative solution is to use a [data provider](LINK TO PUBLIC DOCS) to fetch this information. The dynamic import approach below is an anti-pattern and should be avoided.
|
|
||
| Example of **correct** code: | ||
|
|
||
| Render all form factor outputs and show/hide with media queries https://jsbin.com/gasuyidixa/edit?html,css,output. |
There was a problem hiding this comment.
Linking to a third party host we don't control makes this doc more fragile than eg inlining the example. Please revise.
There was a problem hiding this comment.
@lpomerleau , could you help me with the suggestion as the one from the internal doc points to whatever I have in my PR?
There was a problem hiding this comment.
The public doc hasn't been updated to show a form factor example for 254 yet. Eventually, it will be here: https://developer.salesforce.com/docs/platform/lwr/guide/lwr-portable-best-practices.html
I'm not sure what to do in the meantime.
| - `@salesforce/customPermission/*` | ||
| - `@salesforce/featureFlag/*` | ||
|
|
||
| To replace these deprecated modules, it is recommended to use a **data provider** to fetch the required information declaratively. Avoid using the dynamic import pattern as shown below, as it is considered an **anti-pattern**. |
There was a problem hiding this comment.
There is a suggestion here for a link to use data providers: #164 (comment)
@nrobertdehault Is there public doc for the User data binding we can use here?
There was a problem hiding this comment.
I think we definitely need at least a link to the documentation and/or an example.
There was a problem hiding this comment.
There's no data provider for perms AFAIK and the users ones are not SSRable yet.
The section below recommends using a dynamic import, can we stick to that for now?
There was a problem hiding this comment.
@lpomerleau , are you OK to stick with dynamic imports for now?
There was a problem hiding this comment.
The word "deprecated" refers to an API that is marked for future removal but still works for now. In this case, the issue seems to be more about the availability of the API in different environments.
There was a problem hiding this comment.
Here is the documentation for available expressions: https://developer.salesforce.com/docs/atlas.en-us.exp_cloud_lwr.meta/exp_cloud_lwr/advanced_expressions.htm
There was a problem hiding this comment.
are you OK to stick with dynamic imports for now?
Yes. It's the only reasonable workaround
lib/rules/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Laura <49494194+lpomerleau@users.noreply.github.com>
…-ssrable-components.js Co-authored-by: Laura <49494194+lpomerleau@users.noreply.github.com>
docs/rules/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components.md
Outdated
Show resolved
Hide resolved
lib/rules/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components.js
Outdated
Show resolved
Hide resolved
…-ssrable-components.js Co-authored-by: nrobertdehault <nrobertdehault@salesforce.com>
Shinoni
force-pushed
the
eslint-no-import-scoped-modules
branch
from
c10a948 to
e963a3e
Compare
Shinoni
requested review from
lpomerleau,
kevinv11n,
nrobertdehault,
khangsfdc,
tedconn and
pmdartus
|
Feedback from @wjhsf In the eslint plugin, there’s currently no consistency in the SSR rule names:
To have a more consistent naming convention (and terser names), please change all of them to the format For the few older ones that have already been published, we could either do a rename and release a major, or we could keep the old ones as aliases, but mark them as deprecated. |
README.md
Outdated
| | [lwc/valid-graphql-wire-adapter-callback-parameters](./docs/rules/valid-graphql-wire-adapter-callback-parameters.md) | ensure graphql wire adapters are using 'errors' instead of 'error' | | | ||
| | [lwc/no-host-mutation-in-connected-callback](./docs/rules/no-host-mutation-in-connected-callback.md) | disallow the host element mutation in 'connectedCallback' | | | ||
| | [lwc/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components](./docs/rules/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components.md) | disallow static imports of user-specific scoped modules in SSR-able components | | | ||
| | [lwc/no-form-factor-in-ssrable-components](./docs/rules/no-form-factor-in-ssrable-components.md) | disallow formFactor in SSR-able components | | |
There was a problem hiding this comment.
| | [lwc/no-form-factor-in-ssrable-components](./docs/rules/no-form-factor-in-ssrable-components.md) | disallow formFactor in SSR-able components | | | |
| | [lwc/ssr/no-form-factor](./docs/rules/no-form-factor-in-ssrable-components.md) | disallow formFactor in SSR-able components | | |
README.md
Outdated
| | [lwc/no-node-env-in-ssr](./docs/rules/no-node-env-in-ssr.md) | disallow usage of process.env.NODE_ENV in SSR | | | ||
| | [lwc/valid-graphql-wire-adapter-callback-parameters](./docs/rules/valid-graphql-wire-adapter-callback-parameters.md) | ensure graphql wire adapters are using 'errors' instead of 'error' | | | ||
| | [lwc/no-host-mutation-in-connected-callback](./docs/rules/no-host-mutation-in-connected-callback.md) | disallow the host element mutation in 'connectedCallback' | | | ||
| | [lwc/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components](./docs/rules/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components.md) | disallow static imports of user-specific scoped modules in SSR-able components | | |
There was a problem hiding this comment.
| | [lwc/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components](./docs/rules/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components.md) | disallow static imports of user-specific scoped modules in SSR-able components | | | |
| | [lwc/ssr/no-static-imports-of-user-specific-scoped-modules](./docs/rules/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components.md) | disallow static imports of user-specific scoped modules in SSR-able components | | |
| @@ -0,0 +1,74 @@ | |||
| # Using @salesforce/client/formFactor in SSR-able components is not the best practice(`lwc/no-form-factor-in-ssrable-components`) | |||
There was a problem hiding this comment.
| # Using @salesforce/client/formFactor in SSR-able components is not the best practice(`lwc/no-form-factor-in-ssrable-components`) | |
| # Using @salesforce/client/formFactor in SSR-able components is not the best practice(`lwc/ssr/no-form-factor`) |
lib/index.js
Outdated
| 'no-static-imports-of-user-specific-scoped-modules-in-ssrable-components': require('./rules/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components'), | ||
| 'no-form-factor-in-ssrable-components': require('./rules/no-form-factor-in-ssrable-components'), |
There was a problem hiding this comment.
| 'no-static-imports-of-user-specific-scoped-modules-in-ssrable-components': require('./rules/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components'), | |
| 'no-form-factor-in-ssrable-components': require('./rules/no-form-factor-in-ssrable-components'), | |
| 'ssr/no-static-imports-of-user-specific-scoped-modules': require('./rules/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components'), | |
| 'ssr/no-form-factor': require('./rules/no-form-factor-in-ssrable-components'), |
There was a problem hiding this comment.
Feedback from @wjhsf
In the eslint plugin, there’s currently no consistency in the SSR rule names:
lwc/no-restricted-browser-globals-during-ssrlwc/no-unsupported-ssr-propertieslwc/no-node-env-in-ssrlwc/no-unsuppoted-node-api-in-ssrable-componentslwc/no-unguarded-async-event-in-ssrlwc/no-form-factor-in-ssrable-componentslwc/no-static-imports-of-user-specific-scoped-modules-in-ssrable-components
To have a more consistent naming convention (and terser names), please change all of them to the format lwc/ssr/*.
For rule and doc files names, Either "ssr/" or "ssr-" is probably fine. It depends on what eslint complains about, since it likes things to match the rule names.
For the few older ones that have already been published, we could either do a rename and release a major, or we could keep the old ones as aliases, but mark them as deprecated.
No description provided.