@@W-21951736@@ - Integrate MRT Data Access Layer - #3787

Should we test the opposite that the MRT data store is disabled by default?

We can keep this test but I think we want to make sure to override the default value to make sure the flag is enabled before running the test:

 process.env.PWAKIT_MRT_DATA_STORE_ENABLED = 'true'

I assume we don't want to commit these plan md files.

Correct. I'll be cleaning up a few files before merging.

I assume we don't want to commit these plan md files.

Is this the best pattern to follow to integrate the data-store feature directly in the react-render pipeline gated using a flag?

A cleaner solution could be to follow the HoC model we use with withReactQuery and withLegacyGetProps. We could create a withMrtDataStore HoC, and use it like withReactQuery(withMrtDataStore(AppConfig), options).

I wouldn't necessarily say that is a cleaner solution, per say. Using an HoC like data fetching was done for a different reason, which we don't have in the data store requirement. (withReactQuery/withLegacyGetProps was reacting an abstraction to support multiple data providers, we don't have that here, and probably won't ever have that.)

If we wanted to implement this similar to how we are implementing the data fetching we would still have to modify the react-rending pipeline anyway. As mentioned previously the hoc + doInitAppState is an abstraction that allows use to have multiple data providers for state in the application. That isn't what we are trying to achieve here with the MRT data store implementation, e.g. we aren't going to have another provider/source of data store data.

I'm not convinced that what you are suggesting is something we should look into, primarily because it would be extra effort and we don't have intentions of having a second means of applying data store data, so it would probably be overkill.

The main idea is to avoid introducing the pattern of making direct changes to the react rendering but I see the HoC could be overkill. Should we at least extract the data store logic into a helper to make the performRender easier to read?

Do we still need to confirm these constants with MRT? Can we clean the comments?

I didn't even know those "* Confirm with MRT." comments where in there. No need to confirm, we know those are the values.

nit: Should we use here the caret for the version?

I think that was pinned because that is the current convention in sf-next and that is the implementation AI was referenced to. I can update that to be more in align with the pwa-kit convensions.

Should we add a conditional to make sure we have a value before caching? if the first request runs into a race condition where runs befroe the env setup we could end with and empty cached provider.

That change would stop us from reusing the cached local/no-op provider when the MRT env trio isn’t set—which is exactly when we still need that cache. In practice Lambda already has env before the handler runs, and tests can reset the cache with resetDataStoreProviderCacheForTests() when they flip env.

Should we print a warning log for the key that we are returning an empty value for debugging?

Sure. I can add some logging.

The main idea is to avoid introducing the pattern of making direct changes to the react rendering but I see the HoC could be overkill. Should we at least extract the data store logic into a helper to make the performRender easier to read?

Does using require.resolve here works instead of reaching into ../node_modules/@salesforce/mrt-utilities/..?

E.g.:

const mrtMiddlewareDataStore = require.resolve(
    '@salesforce/mrt-utilities/dist/esm/middleware/data-store.js'
)

I don't think there would be much of a gain in using that that method. This is what the change would look like as per AI:

## Files affected

| File | Role |
|------|------|
| `packages/pwa-kit-runtime/jest.config.js` | **Only** file that needs an edit. |

Nothing else has to change: imports, tests, and other packages keep the same public specifiers; only how Jest resolves the mapper target changes.

---

## What the change looks like

Introduce one resolved path and reuse it for both entries that currently point at the same file:

```javascript
// eslint-disable-next-line @typescript-eslint/no-var-requires
const base = require('internal-lib-build/configs/jest/jest.config')

// eslint-disable-next-line @typescript-eslint/no-var-requires
const mrtMiddlewareDataStore = require.resolve(
    '@salesforce/mrt-utilities/dist/esm/middleware/data-store.js'
)

module.exports = {
    ...base,
    moduleNameMapper: {
        ...base.moduleNameMapper,
        '^@h4ad/serverless-adapter/lib/(.*)$':
            '<rootDir>/node_modules/@h4ad/serverless-adapter/lib/$1/index.cjs',
        '^@salesforce/mrt-utilities/data-store$': mrtMiddlewareDataStore,
        '^@salesforce/mrt-utilities/middleware$': mrtMiddlewareDataStore,
        // ... rest unchanged
    },
    // ...
}

require.resolve returns an absolute path; Jest’s moduleNameMapper accepts that.

If ESLint complains about require.resolve, mirror the same no-var-requires disable you already use for require('internal-lib-build/...') (as in the snippet).

Are we leaving this demo page in the final template intentionally or was just for the review?

That was only for the review. It ill be removed when I get the +1

It would be nice to know how much performance tax are we paying...

Is it possible to add server timing entries for this?

Good call out. I've update the code to have performance marks for initialization of the store and its fetching.

Here is what I have found:

• Cold start fetch is about 65ms, with subsequent fetches being 7ms

Please note that this is the reason this feature is opt-in and not opt-out. I expect customers site and global preferences to be a lot larger than our out of the box sites'.

I've update the doc ticket to include an AC covering this topic.

I feel like something is wrong with our monorepo build pipeline, we shouldn't need to do import via relative paths, but that may require some work inside the monorepo and not a blocking comment for your PR