Improve loading

[hatemhosny]

What type of PR is this? (check all applicable)

• ✨ Feature
• 🐛 Bug Fix
• 📝 Documentation Update
• 🎨 Style
• ♻️ Code Refactor
• 🔥 Performance Improvements
• ✅ Test
• 🤖 Build
• 🔁 CI
• 📦 Chore (Release)
• ⏩ Revert
• 🌐 Internationalization / Translation

Description

This PR improves loading new projects.

A project config can be generated from a variety of sources. Some of the sources are already available when loading (e.g. SDK config, params, compressed code in x param, etc), while others need to be imported (e.g. template, import URL, config content URL, etc)

These sources were mixed in importExternalContent. So, initializePlayground started loading the UI then followed by a flash of changing content when importExternalContent runs.
In addition, a notification of "Loading Content" appeared even when the content was already available.

This PR now handles all already available sources in the initial load and keeps only remote content to importExternalContent.

@BassemHalim whould you please review this, which is very much related to your PR #819

[netlify]

✅ Deploy Preview for livecodes ready!

Name	Link
🔨 Latest commit	77854e8
🔍 Latest deploy log	https://app.netlify.com/projects/livecodes/deploys/6830fc7c1804f300082db19d
😎 Deploy Preview	https://deploy-preview-827--livecodes.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[hatemhosny]

This PR drops a very old (and undocumented) feature that allows the playground URL hash to act as an import URL. This has been replaced by the x query param before the first available permanent URL version.

This kept conflicting with the new hash params introduced in #819 and was difficult (and not worth it) to differentiate with this refactor.

This is reverted and is still backward-compatible.

[BassemHalim]

Hi Dr.@hatemhosny
So I reviewed the PR and everything looks good. My only comment is if we need the new sdkConfig parameter in importExternalContent; the only time it is passed is in initializePlayground and in that function we do the following

//core.ts:5410
 ...
  const sdkConfig = importCompressedCode(params.config ?? '');
  const initialConfig = { ...codeImportConfig, ...appConfig, ...sdkConfig };// <== includes sdkConfig
 ...
  setConfig(buildConfig({ ...getConfig(), ...userConfig, ...initialConfig })); // <== includes initialConfig
 ...
 importExternalContent({
    config: getConfig(), // <== this should have the fields in initialConfig so we shouldn't really need sdkConfig
    sdkConfig,
    configUrl: params.config,
    template: params.template,
    importUrl: Object.keys(codeImportConfig).length > 0 ? '' : params.x, // do not re-import compressed code
  }).then(async (contentImported) => {

so I think the props in sdkConfig should already be in config and we wouldn't need that new parameter.

(P.S. I am still looking at the failing tests)

[cloudflare-workers-and-pages]

Deploying livecodes with    Cloudflare Pages

Latest commit:	77854e8
Status:	✅  Deploy successful!
Preview URL:	https://782d9676.livecodes.pages.dev
Branch Preview URL:	https://improve-loading-config.livecodes.pages.dev

View logs

[hatemhosny]

Thank you @BassemHalim for the review.

so I think the props in sdkConfig should already be in config and we wouldn't need that new parameter.

Yes, sdkConfig was already added. However, it is passed separately to importExternalContent to preserve precedence when multiple sources are used. SDK config properties should override those in template and import. We need sdkConfig to be able to do this:

  await loadConfig(
    buildConfig({
      ...config,
      ...templateConfig,
      ...importUrlConfig,
      ...configUrlConfig,
      ...sdkConfig,
      ...contentUrlConfig,
    }),
    parent.location.href,
    false,
  );

We cannot use config for the override because this also has the defaultConfig (from buildConfig).

Example:

const options: EmbedOptions = {
  template: 'react',
  config: {
    activeEditor: 'markup',
    markup: {
      language: 'html',
      content: '<p>Hello World (from config)</p>',
    },
  },
  params: {
    console: 'open',
  },
};

demo: https://livecodes.io/?x=id/i8628qhyin6

(P.S. I am still looking at the failing tests)

These were related to a feature to use hash as import url (now replaced by x param).
This was an old feature which was replaced by the query param very early on and did not even make it to the docs.
I removed this feature because it kept clashing with the new hash params.
However, there are tests that depend on this. So, I thought maybe someone else might have also been using it.
It felt bad to change the tests just to pass CI, so I reverted the change, and added a workaround to stay backward-compatible!
Now the tests pass.

This PR still needs some tests for different scenarios and different sources.
I will add these before I merge it.

[BassemHalim]

oh ok I see what you mean.
I want to point out one thing, so if a template was used and the config param overwrites it, it would only partially overwrite it for example here https://livecodes.io/?x=id/t879rd4df3b if we set template to markdown then overwrite the markup with html it would overwrite the markup only and not the css or js. This still technically falls within the overwrite rules and is valid but I just wanted to point that out in case that is not the intended behavior but I think this is fine.

[hatemhosny]

Thank you @BassemHalim

Yes, this is indeed the intended behaviour.
In your example, we may use the setup (e.g. styles, importmaps, scripts, ...etc) and just add content.
This can be significant (e.g. Shadcn UI template)

Also this allows just adding query params to a share URL to modify some behaviours. e.g. mode=result or console=open

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[hatemhosny]

added e2e tests for SDK options.

I think this is ready now to merge.