Skip to content

Comments

Add support for auto-discovering fallback content#320

Merged
marcospassos merged 4 commits intomasterfrom
fallback-autodiscovery
Mar 15, 2025
Merged

Add support for auto-discovering fallback content#320
marcospassos merged 4 commits intomasterfrom
fallback-autodiscovery

Conversation

@marcospassos
Copy link
Member

Summary

This pull request adds support for the SDK to auto-discover fallback content from the @croct/content package generated by the CLI.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@marcospassos marcospassos added the feature New feature label Feb 28, 2025
@pkg-pr-new
Copy link

pkg-pr-new bot commented Feb 28, 2025

Open in Stackblitz

npm i https://pkg.pr.new/@croct/plug@320

commit: 1aeccb0

Copy link
Member

@Fryuni Fryuni left a comment

Choose a reason for hiding this comment

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

Dynamically importing a runtime defined module goes against a lot of work that has been going on the ecosystem. This can break the library for many users.

"compilerOptions": {
"module": "CommonJS",
"target": "ES2019",
"module": "ES2020",
Copy link
Member

Choose a reason for hiding this comment

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

This drops support for everyone using CJS on their projects. I'm in favor of it, CJS is outdated and a anchor dragging us, but just making sure this is a an intended decision.

It might still work for most cases, but TS won't be guaranteeing that so seemingly unrelated changes could suddenly downstream CJS consumers.

Copy link
Member Author

@marcospassos marcospassos Mar 1, 2025

Choose a reason for hiding this comment

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

We need at least ES2020 to preserve the import statement, otherwise, it will be compiled into require, and won't work as intended when bundled since it's synchronous and we need asynchronous importing so the bundler can do code splitting.

Copy link
Member Author

Choose a reason for hiding this comment

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

From my experience, it usually works out of the box, as modern setups support both CJS and ES6 imports. The only exception that typically requires manual configuration is Jest. Do you have any other concerns here?


return promise;
try {
return {content: (await import(`@croct/content/slot/${file}`)).default};
Copy link
Member

@Fryuni Fryuni Mar 1, 2025

Choose a reason for hiding this comment

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

This might cause problems when using the plug with a bundler since they won't be able to resolve the dynamic import.

Vite/Rollup, for example, will straight out refuse to bundle this even with the official dynamic import plugin1 and require a custom plugin to hook into the process and ignore the errors. The value from the fallback won't be loadable from the bundle even with the plugin. It would need to be import(/* @vite-ignore */ `@croct/content/slot/${file}`).

Esbuild will have a similar problem. And maintaining comments to have every bundler ignore this will:

  • Be a pain to maintain;
  • Only make the code fail at runtime instead of during build since the bundle won't have the code for the module

Footnotes

  1. https://github.com/rollup/plugins/tree/master/packages/dynamic-import-vars#limitations

@marcospassos
Copy link
Member Author

marcospassos commented Mar 1, 2025

Dynamically importing a runtime defined module goes against a lot of work that has been going on the ecosystem. This can break the library for many users.

This PR was originally focused on supporting Next.js only, because both Webpack and Turbopack can automatically recognize the path and generate separate chunks for each JSON file. However, I then decided to include support for Plug JS, taking a feature-degradation approach. I didn't anticipate that the builds would fail to compile instead of simply retaining the import statement in the final output. It would have been acceptable for the import to fail at runtime, as the current behavior doesn't change if that happens. Moreover, using import maps would enable this in browsers as well.

I can continue to provide support exclusively for Next.js, but then projects using Webpack won't benefit from this new feature.

PS: I bundled the Plug itself using rollout before proposing it, and it worked just fine. Output:

fetch(e, t = {}) {
    const [n, r = "latest"] = e.split("@"), i = this.sdk.getLogger();
    return this.sdk.contentFetcher.fetch(n, "latest" === r ? t : {...t, version: r}).catch((async o => {
        i.error(`Failed to fetch content for slot "${n}@${r}": ${ni.formatCause(o)}`);
        const s = t.preferredLocale ?? null, a = `${null === s ? "" : `${s}/`}${e}.json`;
        try {
            return {content: (await import(`@croct/content/slot/${a}`)).default}
        } catch {
            throw o
        }
    }))
}

What are your thoughts?

@marcospassos
Copy link
Member Author

marcospassos commented Mar 1, 2025

I created a project using Vite and vanilla TypeScript, and the build process completed without any issues:

image

Upon inspecting the compiled output, the import statement remained intact:

image

As expected, the JSON files were not bundled, though the client did attempt to load them:

TypeError: Failed to resolve module specifier '@croct/content/slot/test.json'
    at @croct_plug.js?v=3cf4ea8c:6696:64
    at async main.ts:12:17

While there may be ways to include these JSON files in the bundle, that isn't the focus of this pull request. My goal is simply to enable auto-discovery of default content for bundlers that support it, without altering the SDK's current behavior.

@marcospassos
Copy link
Member Author

marcospassos commented Mar 2, 2025

After our discussion here, I believe I've come up with a much more robust and widely supported proposal:

All widely used bundlers, such as Vite, Rollup, and Webpack, support dynamic imports. The main issue arises when importing dynamic paths because the bundler has no way of determining which imports will be used at build time. Even if it can partially identify them (as Webpack does), it's tricky to decide how to split them into chunks that load only when needed.

A simple way to address this is to avoid dynamic paths altogether and instead generate a static import map with literal relative paths, which all production-grade bundlers support.

In practical terms:

  • @croct/content provides a stub function that defaults to returning null if the project isn't using the CLI to generate content.
  • The CLI generates the function, mapping all imports.
  • The SDK can then rely on this function to load content.

Here's what the generated module looks like:

const contentMap = {
  "en": {
    "magic-ui-user-reviews": () => import("./en/magic-ui-user-reviews@1.json"),
    "magic-ui-user-reviews@1": () => import("./en/magic-ui-user-reviews@1.json"),
  },
};

const defaultLocale = "en";

export function loadContent(slotId, language = defaultLocale) {
  if (contentMap[language]?.[slotId]) {
    return contentMap[language][slotId]().then((module) => module.default);
  }

  return language !== defaultLocale ? loadContent(slotId) : Promise.resolve(null);
}

You can see here the changes to the @croct/content and here the changes in the generated code.

Now, not only the fallback can be automatically found, but bundlers create chunks for each content and only load them when needed:

image

@marcospassos marcospassos merged commit e138f73 into master Mar 15, 2025
10 checks passed
@marcospassos marcospassos deleted the fallback-autodiscovery branch March 15, 2025 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants