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

Make the SDK friendly to locally link and develop on #693

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
cd007b4
Make the SDK friendly to locally link and develop on
MadLittleMods Feb 26, 2022
8fb2b27
Fix typos pointing to wrong files
MadLittleMods Feb 26, 2022
0023ab3
Add a placeholder for upgrading vite to comment on
MadLittleMods Feb 26, 2022
d247bc4
Merge branch 'master' into madlittlemods/686-682-local-friendly-devel…
MadLittleMods Apr 5, 2022
95d1730
Update Vite which includes fixes to importing `*.js?url` with `exports`
MadLittleMods Apr 5, 2022
dd06d78
Avoid ERR_REQUIRE_ESM errors when requiring SDK
MadLittleMods Apr 5, 2022
2401b7f
Add way to test whether SDK works in ESM and CommonJS
MadLittleMods Apr 6, 2022
12d6447
Merge branch 'master' into madlittlemods/686-682-local-friendly-devel…
MadLittleMods Apr 19, 2022
f61bf60
Enable extended globs for removing all but some filename !(filename)
MadLittleMods Apr 19, 2022
f56dc58
Fix tests after theme updates
MadLittleMods Apr 20, 2022
e9cee2e
Merge branch 'master' into madlittlemods/686-682-local-friendly-devel…
MadLittleMods Apr 20, 2022
f1e07b6
Explain what is being deleted by the strange syntax
MadLittleMods Apr 20, 2022
ce289ba
Remove extra space
MadLittleMods Apr 20, 2022
d053d43
Update Vite to avoid flakey errors in our PostCSS plugins
MadLittleMods May 5, 2022
75098b4
Merge branch 'master' into madlittlemods/686-682-local-friendly-devel…
MadLittleMods May 5, 2022
e54482e
Add some comments
MadLittleMods May 5, 2022
639358b
Merge branch 'master' into madlittlemods/686-682-local-friendly-devel…
MadLittleMods May 12, 2022
b725269
Clean up index.html in the right spot
MadLittleMods May 18, 2022
1b2a6b5
Merge branch 'master' into madlittlemods/686-682-local-friendly-devel…
bwindels May 30, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions package.json
Expand Up @@ -11,9 +11,11 @@
"lint-ci": "eslint src/",
"test": "impunity --entry-point src/platform/web/main.js src/platform/web/Platform.js --force-esm-dirs lib/ src/ --root-dir src/",
"test:postcss": "impunity --entry-point scripts/postcss/tests/css-compile-variables.test.js scripts/postcss/tests/css-url-to-variables.test.js",
"test:sdk": "cd ./scripts/sdk/test/ && yarn --no-lockfile && node test-sdk-in-esm-vite-build-env.js && node test-sdk-in-commonjs-env.js",
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
"start": "vite --port 3000",
"build": "vite build",
"build:sdk": "./scripts/sdk/build.sh"
"build:sdk": "./scripts/sdk/build.sh",
"watch:sdk": "./scripts/sdk/build.sh && yarn run vite build -c vite.sdk-lib-config.js --watch"
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
},
"repository": {
"type": "git",
Expand Down Expand Up @@ -47,7 +49,7 @@
"regenerator-runtime": "^0.13.7",
"text-encoding": "^0.7.0",
"typescript": "^4.3.5",
"vite": "^2.6.14",
"vite": "^2.9.1",
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
"xxhashjs": "^0.2.2"
},
"dependencies": {
Expand Down
2 changes: 1 addition & 1 deletion scripts/postcss/css-url-processor.js
Expand Up @@ -39,7 +39,7 @@ function colorsFromURL(url, colorMap) {
function processURL(decl, replacer, colorMap) {
const value = decl.value;
const parsed = valueParser(value);
parsed.walk(async node => {
parsed.walk(node => {
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
if (node.type !== "function" || node.value !== "url") {
return;
}
Expand Down
2 changes: 1 addition & 1 deletion scripts/postcss/svg-colorizer.js
Expand Up @@ -37,7 +37,7 @@ module.exports.buildColorizedSVG = function (svgLocation, primaryColor, secondar
if (svgCode === coloredSVGCode) {
throw new Error("svg-colorizer made no color replacements! The input svg should only contain colors #ff00ff (primary, case-sensitive) and #00ffff (secondary, case-sensitive).");
}
const fileName = svgLocation.match(/.+\/(.+\.svg)/)[1];
const fileName = svgLocation.match(/.+[/\\](.+\.svg)/)[1];
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
const outputName = `${fileName.substring(0, fileName.length - 4)}-${createHash(coloredSVGCode)}.svg`;
const outputPath = path.resolve(__dirname, "../../.tmp");
try {
Expand Down
16 changes: 14 additions & 2 deletions scripts/sdk/base-manifest.json
Expand Up @@ -2,6 +2,18 @@
"name": "hydrogen-view-sdk",
"description": "Embeddable matrix client library, including view components",
"version": "0.0.10",
"main": "./hydrogen.es.js",
"type": "module"
"main": "./lib-build/hydrogen.cjs.js",
"exports": {
".": {
"import": "./lib-build/hydrogen.es.js",
"require": "./lib-build/hydrogen.cjs.js"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Referencing hydrogen.es.js directly in the build, ./lib-build/hydrogen.es.js, to make it compatible with watching for an local changes and rebuild: yarn run vite build -c vite.sdk-lib-config.js --watch

},
"./paths/vite": "./paths/vite.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This export isn't used but it's part of the ./scripts/sdk/transform-paths.js ./src/platform/web/sdk/paths/vite.js ./target/paths/vite.js stuff in build.sh that's currently commented out.

"./style.css": "./asset-build/assets/theme-element-light.css",
"./theme-element-light.css": "./asset-build/assets/theme-element-light.css",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we avoid this if we just say in the SDK docs that the path to the css file is hydrogen-view-sdk/assets/theme-element-light.css rather than hydrogen-view-sdk/theme-element-light.css. Will be easier towards the future when adding more assets. Probably best to keep style.css for now for backwards compat though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we avoid this if we just say in the SDK docs that the path to the css file is hydrogen-view-sdk/assets/theme-element-light.css rather than hydrogen-view-sdk/theme-element-light.css. Will be easier towards the future when adding more assets.

Yes, we can! Should we do that in this PR or in a follow-up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 👍 reaction is ambiguous on what option to do so I'm going to opt to fix this in a fast follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

wfm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #746

"./theme-element-dark.css": "./asset-build/assets/theme-element-dark.css",
"./main.js": "./asset-build/assets/main.js",
"./download-sandbox.html": "./asset-build/assets/download-sandbox.html",
"./assets/*": "./asset-build/assets/*"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix #682

Make the assets path available to reference and serve separately.

app.use(express.static(path.dirname(require.resolve('hydrogen-view-sdk/assets/main.js'))));

}
}
16 changes: 3 additions & 13 deletions scripts/sdk/build.sh
@@ -1,5 +1,5 @@
#!/bin/bash
rm -rf target
rm -rf target/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only remove the directory contents instead of the whole directory to maintain the npm link/yarn link symlink

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added as a comment to the script

yarn run vite build -c vite.sdk-assets-config.js
yarn run vite build -c vite.sdk-lib-config.js
yarn tsc -p tsconfig-declaration.json
Expand All @@ -10,17 +10,7 @@ mkdir target/paths
cp doc/SDK.md target/README.md
pushd target
pushd asset-build/assets
mv main.*.js ../../main.js
# Create a copy of light theme for backwards compatibility
cp theme-element-light.*.css ../../style.css
# Remove asset hash from css files
mv theme-element-light.*.css ../../theme-element-light.css
mv theme-element-dark.*.css ../../theme-element-dark.css
mv download-sandbox.*.html ../../download-sandbox.html
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of needing to move these 3 files and rename them without the hash here, we handle removing the hash in vite.sdk-assets-config.js (rollupOptions.output.assetFileNames) and reference them directly in place in the package.json exports

rm *.js *.wasm
mv ./* ../../
rm !(main).js *.wasm
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
popd
rm -rf asset-build
mv lib-build/* .
rm -rf lib-build
rm index.html
popd
16 changes: 1 addition & 15 deletions scripts/sdk/create-manifest.js
Expand Up @@ -3,21 +3,7 @@ const fs = require("fs");
const appManifest = require("../../package.json");
const baseSDKManifest = require("./base-manifest.json");
/*
need to leave exports out of base-manifest.json because of #vite-bug,
with the downside that we can't support environments that support
both esm and commonjs modules, so we pick just esm.
```
"exports": {
".": {
"import": "./hydrogen.es.js",
"require": "./hydrogen.cjs.js"
},
"./paths/vite": "./paths/vite.js",
"./style.css": "./style.css"
},
```

Also need to leave typescript type definitions out until the
Need to leave typescript type definitions out until the
typescript conversion is complete and all imports in the d.ts files
exists.
```
Expand Down
3 changes: 3 additions & 0 deletions scripts/sdk/test/.gitignore
@@ -0,0 +1,3 @@
node_modules
dist
yarn.lock
2 changes: 2 additions & 0 deletions scripts/sdk/test/deps.d.ts
@@ -0,0 +1,2 @@
// Keep TypeScripts from complaining about hydrogen-view-sdk not having types yet
declare module "hydrogen-view-sdk";
21 changes: 21 additions & 0 deletions scripts/sdk/test/esm-entry.ts
@@ -0,0 +1,21 @@
import * as hydrogenViewSdk from "hydrogen-view-sdk";
import downloadSandboxPath from 'hydrogen-view-sdk/download-sandbox.html?url';
import workerPath from 'hydrogen-view-sdk/main.js?url';
import olmWasmPath from '@matrix-org/olm/olm.wasm?url';
import olmJsPath from '@matrix-org/olm/olm.js?url';
import olmLegacyJsPath from '@matrix-org/olm/olm_legacy.js?url';
const assetPaths = {
downloadSandbox: downloadSandboxPath,
worker: workerPath,
olm: {
wasm: olmWasmPath,
legacyBundle: olmLegacyJsPath,
wasmBundle: olmJsPath
}
};
import "hydrogen-view-sdk/style.css";

console.log('hydrogenViewSdk', hydrogenViewSdk);
console.log('assetPaths', assetPaths);

console.log('Entry ESM works ✅');
12 changes: 12 additions & 0 deletions scripts/sdk/test/index.html
@@ -0,0 +1,12 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Vite App</title>
</head>
<body>
<div id="app" class="hydrogen"></div>
<script type="module" src="./esm-entry.ts"></script>
</body>
</html>
8 changes: 8 additions & 0 deletions scripts/sdk/test/package.json
@@ -0,0 +1,8 @@
{
"name": "test-sdk",
"version": "0.0.0",
"description": "",
"dependencies": {
"hydrogen-view-sdk": "link:../../../target"
}
}
13 changes: 13 additions & 0 deletions scripts/sdk/test/test-sdk-in-commonjs-env.js
@@ -0,0 +1,13 @@
// Make sure the SDK can be used in a CommonJS environment.
// Usage: node scripts/sdk/test/test-sdk-in-commonjs-env.js
const hydrogenViewSdk = require('hydrogen-view-sdk');

// Test that the "exports" are available:
// Worker
require.resolve('hydrogen-view-sdk/main.js');
// Styles
require.resolve('hydrogen-view-sdk/style.css');
// Can access files in the assets/* directory
require.resolve('hydrogen-view-sdk/assets/main.js');

console.log('SDK works in CommonJS ✅');
19 changes: 19 additions & 0 deletions scripts/sdk/test/test-sdk-in-esm-vite-build-env.js
@@ -0,0 +1,19 @@
const { resolve } = require('path');
const { build } = require('vite');

async function main() {
await build({
outDir: './dist',
build: {
rollupOptions: {
input: {
main: resolve(__dirname, 'index.html')
}
}
}
});

console.log('SDK works in Vite build ✅');
}

main();
20 changes: 20 additions & 0 deletions vite.sdk-assets-config.js
Expand Up @@ -3,11 +3,31 @@ const mergeOptions = require('merge-options');
const themeBuilder = require("./scripts/build-plugins/rollup-plugin-build-themes");
const {commonOptions, compiledVariables} = require("./vite.common-config.js");

const pathsToExport = [
"main.js",
"download-sandbox.html",
"theme-element-light.css",
"theme-element-dark.css",
];

export default mergeOptions(commonOptions, {
root: "src/",
base: "./",
build: {
outDir: "../target/asset-build/",
rollupOptions: {
output: {
assetFileNames: (chunkInfo) => {
// Get rid of the hash so we can consistently reference these
// files in our `package.json` `exports`
if(pathsToExport.includes(path.basename(chunkInfo.name))) {
return "assets/[name].[ext]";
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of renaming these files in build.sh, we can just give them the proper name as part of the Vite build.


return "assets/[name]-[hash][extname]";
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference, docs for the rollupOptions.output.assetFileNames option, https://rollupjs.org/guide/en/#outputassetfilenames

}
}
},
plugins: [
themeBuilder({
Expand Down