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

feat: add dynamic imports for .json assets #2740

Merged
merged 42 commits into from
Feb 12, 2021
Merged

feat: add dynamic imports for .json assets #2740

merged 42 commits into from
Feb 12, 2021

Conversation

pskelin
Copy link
Contributor

@pskelin pskelin commented Jan 28, 2021

TL;DR

Change the default way asset discovery and loading works from static ES6 imports of .json files to dynamic ES6 imports of .json files.

Current implementation

Up until now, importing the @ui5/webcomponents/dist/Assets.js module includes transitively imports of the actual assets like:
import en from "../assets/i18n/messagebundle_en.json";

Since .json imports are not supported by the browser, this requires a build plugin, which could do two things:

  1. inline the content of the resource in the variable en
  2. copy the resource in the dist folder and put a URL reference in the variable en

approach 1 should not be considered productive, as all assets used by ui5-webcomponents get inlined, but only one is used at runtime. This produces huge JS bundles.
approach 2 is what is recommended for productive usage, as ui5-webcomponents will fetch just the current asset that is necessary.

Issues with current implementation

This approach works with rollup and webpack, where it can be configured to treat JSON imports as URL imports. There are however newer tools like snowpack, vitejs, that correctly assume a static import of a JSON file should return the content of the JSON file and JSON imports cannot be configured as URLs. This is what the browser will do in the future as soon as the JSON module imports land in the HTML spec.

New implementation

A new way of exposing the assets is introduced with this change, changing the above form of static JSON imports to dynamic JSON imports. For dynamic imports, it is expected that the JSON object is inlined, as only one will be retrieved at runtime.
case "en": return (await import("../assets/i18n/messagebundle_en.json")).default;

Public APIs

BREAKING CHANGE: 🚨 dynamic imports of JSON resources by default
The existing import @ui5/webcomponents/dist/Assets.js now contains dynamic imports transitively.

In case you had configured JSON imports to be treated as URLs, this will break as the framework will no longer fetch the URLs. Change back the build configuration to inline JSON files (for example @rollup/plugin-json)

The old way of packaging assets is moved to @ui5/webcomponents/dist/Assets-static.js. This is not a future proof way to reference assets, so it is kept for backwards compatibility or more advanced use cases, should dynamic imports not be available for some reason.

Custom asset configuration

Used when applications want to add more assets or overwrite the existing ones provided by ui5-webcomponents.

i18n

BREAKING CHANGE: 🚨 the method registerI18nBundle of the @ui5/webcomponents-base/asset-registries/i18n.js module is deprecated in favour of registerI18nLoader

PropertiesSupport

BREAKING CHANGE: 🚨 the framework no longer tries to convert .properties files internally and the PropertiesFormatSupport feature is removed. This has to be done by a custom loader using the @ui5/webcomponents-base/dist/PropertiesFileFormat.js module.

Migration

OLD:

import { registerI18nBundle } from "@ui5/webcomponents-base/dist/asset-registries/i18n.js";
import "@ui5/webcomponents-base/dist/features/PropertiesFormatSupport.js";
registerI18nBundle("@ui5/webcomponents", {
	bg: "./lang/messagebundle_bg.properties",
	fr: "./lang/fr.json",
});

NEW:

import { registerI18nLoader } from "@ui5/webcomponents-base/dist/asset-registries/i18n.js";
import parse from "@ui5/webcomponents-base/dist/PropertiesFileFormat.js";

const bgUrl = "./lang/messagebundle_bg.properties";
const frUrl = "./lang/fr.json";

registerI18nLoader("@ui5/webcomponents", "bg", async (localeId) => {
	const props = await (await fetch(bgUrl)).text();
	return parse(props);
});
registerI18nLoader("@ui5/webcomponents", "fr", async (localeId) => {
	return await (await fetch(frUrl)).json();
});

packages/base/bundle.esm.js Outdated Show resolved Hide resolved
packages/base/src/AssetRegistry.js Show resolved Hide resolved
packages/base/src/asset-registries/Icons.js Outdated Show resolved Hide resolved
packages/base/src/asset-registries/LocaleData.js Outdated Show resolved Hide resolved
packages/base/src/asset-registries/i18n.js Show resolved Hide resolved
packages/base/src/asset-registries/i18n.js Outdated Show resolved Hide resolved
packages/base/src/util/parseProperties.js Show resolved Hide resolved

registerIconBundle("SAP-icons-TNT", SAPIcons);
registerIconLoader("SAP-icons-TNT", loadIconsBundle);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the part I mentioned in my previous comment - IMO we must both register the loader and run it explicitly here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like an explicit load after the registration is a good API. This load should happen when an icon is rendered and an explicit load should be up to the app developer. Can we change this in another change, as the previous JSON behaviour was like this as well?

Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

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

APIs:

  • see my comments in the code review

Docs:

  • Public Module Imports.md
  • Assets.md
  • dev/Developing Web Components.md
  • i18n.md

The new package generation script must also be updated:

  • packages/tools/lib/init-package/resources/src/Assets.js

import { registerI18nLoader } from "./asset-registries/i18n.js";
import { registerLocaleDataLoader } from "./asset-registries/LocaleData.js";
import { registerThemePropertiesLoader } from "./asset-registries/Themes.js";
import { registerIconLoader } from "./asset-registries/Icons.js";
Copy link
Contributor

Choose a reason for hiding this comment

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

registerIconLoader -> registerIconsLoader to be consistent with the rest (Data, Properties) which are plural

packages/base/src/asset-registries/Icons.js Outdated Show resolved Hide resolved
@pskelin pskelin changed the title Wip dynamic assets feat: add dynamic imports for .json assets Feb 12, 2021
@vladitasev vladitasev marked this pull request as ready for review February 12, 2021 08:36
@pskelin pskelin merged commit 46e38fb into master Feb 12, 2021
@pskelin pskelin deleted the wip-dynamic-assets branch February 12, 2021 09:35
@vladitasev vladitasev mentioned this pull request May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants