-
Notifications
You must be signed in to change notification settings - Fork 130
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
Changes from 14 commits
cd007b4
8fb2b27
0023ab3
d247bc4
95d1730
dd06d78
2401b7f
12d6447
f61bf60
f56dc58
e9cee2e
f1e07b6
ce289ba
d053d43
75098b4
e54482e
639358b
b725269
1b2a6b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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": "yarn build:sdk && cd ./scripts/sdk/test/ && yarn --no-lockfile && node test-sdk-in-esm-vite-build-env.js && node test-sdk-in-commonjs-env.js", | ||
"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", | ||
|
@@ -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.8", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updating Vite to include fix from vitejs/vite#7098 which allows And to includes fixes for #722 which were added in vitejs/vite#7822 ->vitejs/vite#7827 ( |
||
"xxhashjs": "^0.2.2" | ||
}, | ||
"dependencies": { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Referencing |
||
}, | ||
"./paths/vite": "./paths/vite.js", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This export isn't used but it's part of the |
||
"./style.css": "./asset-build/assets/theme-element-light.css", | ||
"./theme-element-light.css": "./asset-build/assets/theme-element-light.css", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, we can! Should we do that in this PR or in a follow-up? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wfm There was a problem hiding this comment. Choose a reason for hiding this commentThe 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/*" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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')))); |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,10 @@ | |
# Exit whenever one of the commands fail with a non-zero exit code | ||
set -e | ||
set -o pipefail | ||
# Enable extended globs so we can use the `!(filename)` glob syntax | ||
shopt -s extglob | ||
|
||
rm -rf target | ||
rm -rf target/* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -14,17 +16,8 @@ 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
rm *.js *.wasm | ||
mv ./* ../../ | ||
# Remove all `*.wasm` and `*.js` files except for `main.js` | ||
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
node_modules | ||
dist | ||
yarn.lock |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
// Keep TypeScripts from complaining about hydrogen-view-sdk not having types yet | ||
declare module "hydrogen-view-sdk"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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/theme-element-light.css"; | ||
|
||
console.log('hydrogenViewSdk', hydrogenViewSdk); | ||
console.log('assetPaths', assetPaths); | ||
|
||
console.log('Entry ESM works ✅'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"name": "test-sdk", | ||
"version": "0.0.0", | ||
"description": "", | ||
"dependencies": { | ||
"hydrogen-view-sdk": "link:../../../target" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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/theme-element-light.css'); | ||
// Can access files in the assets/* directory | ||
require.resolve('hydrogen-view-sdk/assets/main.js'); | ||
|
||
console.log('SDK works in CommonJS ✅'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]"; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of renaming these files in |
||
|
||
return "assets/[name]-[hash][extname]"; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reference, docs for the |
||
} | ||
} | ||
}, | ||
plugins: [ | ||
themeBuilder({ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is a bit clunky but it makes it easy to test whether
hydrogen-view-sdk
actually works in an ESM Vite build and when usingrequire
in Node.js (CommonJS).All of the "testing" code was added in 2401b7f if you want to review it on its own. Feel free to prompt to remove or different way to test.
Thought the testing was slightly needed as testing manually in each environment is cumbersome. And we really don't know whether the SDK actually works before pushing it out. All of the usage code is just documented in
SDK.md