Make the SDK friendly to locally link and develop on by MadLittleMods · Pull Request #693 · element-hq/hydrogen-web

MadLittleMods · 2022-02-26T07:27:42Z

Make the hydrogen-view-sdk friendly to locally link and develop on. Can now simply yarn watch:sdk to watch and build on changes and use in CommonJS and ESM environments.

Also adds yarn test:sdk to test whether the SDK works in an ESM Vite build and when using require in Node.js (CommonJS).

Fix #686
Fix #682
Fix #722

Instead of deleting the whole target/ directory, leave it alone so the symlink driving the npm link/yarn link stays in tact.

Leave Vite builds in their build directories (/lib-build//asset-build) so you can vite build --watch to build on local changes and still have a consisent place to reference in the package.json exports. Previously, everything relied on build.sh which does a bunch of moving and renaming and made it hard to quickly rebuild on changes.

Add back support for CommonJS (adding the package.json exports).

The last piece is making sure the ?url imports (ex. import workerPath from 'hydrogen-view-sdk/main.js?url';) still work. ~~It looks like this may have just been solved via vitejs/vite#6725 -> vitejs/vite#7073 (literally 2 days ago) and we just need to wait for the next Vite release~~ Was solved by vitejs/vite#7098 and is available in the latest Vite releases 🎉

Also updating vite because it includes fixes for #722 which were added in vitejs/vite#7822 ->vitejs/vite#7827

Dev notes

package.json and exports docs: https://nodejs.org/docs/latest-v17.x/api/packages.html
https://vitejs.dev/config/#build-assetsdir

Use the latest Vite from GitHub source (before release)

How can I try out the latest Vite without waiting for a release?

See https://github.com/vitejs/vite/blob/main/CONTRIBUTING.md#repo-setup to build locally and npm link it

How to yarn install a package from a GitHub Monorepo? This doesn't work for Vite as it requires more build stuff.

See

yarn add https://gitpkg.now.sh/vitejs/vite/packages/vite?main --dev

Fix #686 Fix #682 Instead of deleting the whole `target/` directory, leave it alone so the symlink driving the `npm link`/`yarn link` stays in tact. Leave Vite builds in their build directories (`/lib-build`/`/asset-build`) so you can `vite build --watch` to build on local changes and still have a consisent place to reference in the `package.json` `exports`. Previously, everything relied on `build.sh` which does a bunch of moving and renaming and made it hard to rebuild on changes. Add back support for CommonJS (adding the `package.json` `exports`). The last piece is making sure the `?url` imports (`import workerPath from 'hydrogen-view-sdk/main.js?url';`) work still. It looks like this may have just been solved via vitejs/vite#6725 -> vitejs/vite#7073 (literally 2 days ago) and we just need to wait for the next Vite release 🎉

MadLittleMods · 2022-02-26T08:14:45Z

@@ -1,5 +1,5 @@
 #!/bin/bash
-rm -rf target
+rm -rf target/*


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

Added as a comment to the script

MadLittleMods · 2022-02-26T08:16:59Z

 pushd asset-build/assets
-mv main.*.js ../../main.js
-mv index.*.css ../../style.css
-mv download-sandbox.*.html ../../download-sandbox.html


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

MadLittleMods · 2022-02-26T08:18:15Z

+    "exports": {
+        ".": {
+            "import": "./lib-build/hydrogen.es.js",
+            "require": "./lib-build/hydrogen.cjs.js"


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

MadLittleMods · 2022-02-26T08:19:03Z

+        "./style.css": "./asset-build/assets/index.css",
+        "./main.js": "./asset-build/assets/download-sandbox.html",
+        "./download-sandbox.html": "./asset-build/assets/download-sandbox.html",
+        "./assets/*": "./asset-build/assets/*"


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'))));

MadLittleMods · 2022-02-26T08:20:18Z

+            "import": "./lib-build/hydrogen.es.js",
+            "require": "./lib-build/hydrogen.cjs.js"
+        },
+        "./paths/vite": "./paths/vite.js",


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.

MadLittleMods · 2022-02-26T08:20:58Z

+                  // files in our `package.json` `exports`
+                  if(pathsToExport.includes(path.basename(chunkInfo.name))) {
+                    return "assets/[name].[ext]";
+                  }


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

MadLittleMods · 2022-02-26T08:21:39Z

+                  }
+
+                return "assets/[name]-[hash][extname]";
+              }


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

…opment-and-commonjs Conflicts: package.json scripts/sdk/base-manifest.json

Update to Vite which includes vitejs/vite#7098

…opment-and-commonjs Conflicts: package.json scripts/sdk/base-manifest.json scripts/sdk/build.sh

MadLittleMods · 2022-04-20T05:39:56Z

    "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",


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 using require 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

bwindels

This is fantastic work, thank you so much for all your effort! And sorry for totally missing this PR before.
The changes look great, I just need to run the build on this branch to verify that it all still works, but I'll do that once the blockers are resolved, AIUI:

#721 needs to merge
#722 is should be fixed

I've asked @MidhunSureshR to have a look.

bwindels · 2022-04-20T08:03:01Z

        "./paths/vite": "./paths/vite.js",
-        "./style.css": "./asset-build/assets/index.css",
+        "./style.css": "./asset-build/assets/theme-element-light.css",
+        "./theme-element-light.css": "./asset-build/assets/theme-element-light.css",


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.

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?

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

Addressed in #746 ⏩

…opment-and-commonjs Conflicts: scripts/sdk/build.sh

See #693 (comment)

Fix #722 Updating Vite to includes fixes from vitejs/vite#7822 -> vitejs/vite#7827

MadLittleMods · 2022-05-05T20:04:31Z

    "text-encoding": "^0.7.0",
    "typescript": "^4.3.5",
-    "vite": "^2.6.14",
+    "vite": "^2.9.8",


Updating Vite to include fix from vitejs/vite#7098 which allows import workerPath from 'hydrogen-view-sdk/main.js?url'; to still work even though we have package.json exports defined (vite@2.9.1).

And to includes fixes for #722 which were added in vitejs/vite#7822 ->vitejs/vite#7827 (vite@2.9.8)

…opment-and-commonjs

…opment-and-commonjs Conflicts: scripts/sdk/base-manifest.json

bwindels

Thanks, changes look great, but when running yarn build:sdk or yarn watch:sdk, I get this error:

$ yarn watch:sdk
yarn run v1.22.17
$ ./scripts/sdk/build.sh && yarn run vite build -c vite.sdk-lib-config.js --watch
$ /home/bwindels/dev/hydrogen-web/node_modules/.bin/vite build -c vite.sdk-assets-config.js
Generated an empty chunk: "vendor"
Generated an empty chunk: "index"
$ /home/bwindels/dev/hydrogen-web/node_modules/.bin/vite build -c vite.sdk-lib-config.js
$ /home/bwindels/dev/hydrogen-web/node_modules/.bin/tsc -p tsconfig-declaration.json
~/dev/hydrogen-web/target ~/dev/hydrogen-web
~/dev/hydrogen-web/target/asset-build/assets ~/dev/hydrogen-web/target ~/dev/hydrogen-web
~/dev/hydrogen-web/target ~/dev/hydrogen-web
rm: cannot remove 'index.html': No such file or directory

MadLittleMods · 2022-05-18T05:24:49Z

+pushd target/asset-build
+rm index.html


[...] when running yarn build:sdk or yarn watch:sdk, I get this error:

[...] rm: cannot remove 'index.html': No such file or directory

-- #693 (review)

Fixed this up by removing index.html in the right spot (target/asset-build/index.html).

Sorry didn't pick up on this earlier. I think it's because when running on Windows the git bash window which pops up when you yarn build:sdk to run the shell code closes so fast and masks errors for me. I've tested this on macOS and everything is working now 👍

bwindels

Works now indeed, thanks!

…opment-and-commonjs

MadLittleMods · 2022-05-31T20:25:34Z

Thanks for the review and testing @bwindels 🐗

> Will be easier towards the future when adding more assets. Probably best to keep style.css for now for backwards compat though. > > *-- #693 (comment)

Trying to get this project again after a few months of changes from `hydrogen-view-sdk` and now finally after element-hq/hydrogen-web#693 merged to make the SDK friendly to locally link and develop on.

Get this project running again after a few months of changes from `hydrogen-view-sdk` and now finally after element-hq/hydrogen-web#693 merged to make the SDK friendly to locally link and develop on.

MadLittleMods added the sdk label Feb 26, 2022

MadLittleMods mentioned this pull request Feb 26, 2022

Draft: Scratch changes for matrix-public-archive #653

Draft

MadLittleMods commented Feb 26, 2022

View reviewed changes

Comment thread scripts/sdk/build.sh Outdated

MadLittleMods commented Feb 26, 2022

View reviewed changes

MadLittleMods mentioned this pull request Feb 26, 2022

Re-introduce CommonJS support to SDK #686

Closed

MadLittleMods added 2 commits February 26, 2022 03:08

Fix typos pointing to wrong files

8fb2b27

Add a placeholder for upgrading vite to comment on

0023ab3

MadLittleMods commented Feb 26, 2022

View reviewed changes

Comment thread package.json Outdated

MadLittleMods marked this pull request as draft February 26, 2022 11:23

MadLittleMods changed the title ~~Make the SDK friendly to locally link and develop on~~ Draft: Make the SDK friendly to locally link and develop on Feb 26, 2022

MadLittleMods mentioned this pull request Feb 26, 2022

Draft: Deep import HTML and non-ESM files with exports and ?url query vitejs/vite#7097

Closed

9 tasks

MadLittleMods added 3 commits April 5, 2022 17:15

Merge branch 'master' into madlittlemods/686-682-local-friendly-devel…

d247bc4

…opment-and-commonjs Conflicts: package.json scripts/sdk/base-manifest.json

Update Vite which includes fixes to importing *.js?url with exports

95d1730

Update to Vite which includes vitejs/vite#7098

Avoid ERR_REQUIRE_ESM errors when requiring SDK

dd06d78

MadLittleMods commented Apr 5, 2022

View reviewed changes

Comment thread package.json

MadLittleMods commented Apr 5, 2022

View reviewed changes

Comment thread package.json Outdated

Add way to test whether SDK works in ESM and CommonJS

2401b7f

MadLittleMods changed the title ~~Draft: Make the SDK friendly to locally link and develop on~~ Make the SDK friendly to locally link and develop on Apr 6, 2022

MadLittleMods commented Apr 6, 2022

View reviewed changes

Comment thread package.json Outdated

MadLittleMods marked this pull request as ready for review April 6, 2022 00:30

MadLittleMods requested a review from bwindels April 6, 2022 00:30

Merge branch 'master' into madlittlemods/686-682-local-friendly-devel…

12d6447

…opment-and-commonjs Conflicts: package.json scripts/sdk/base-manifest.json scripts/sdk/build.sh

MadLittleMods commented Apr 19, 2022

View reviewed changes

Comment thread scripts/sdk/build.sh Outdated

MadLittleMods mentioned this pull request Apr 20, 2022

PostCSS css-compile-variables is flakey with vite@2.9.0+ #722

Closed

MadLittleMods commented Apr 20, 2022

View reviewed changes

Comment thread scripts/postcss/css-url-processor.js

MadLittleMods commented Apr 20, 2022

View reviewed changes

Comment thread scripts/postcss/svg-colorizer.js

Fix tests after theme updates

f56dc58

MadLittleMods commented Apr 20, 2022

View reviewed changes

Comment thread package.json Outdated

bwindels reviewed Apr 20, 2022

View reviewed changes

MadLittleMods added 4 commits April 20, 2022 11:58

Merge branch 'master' into madlittlemods/686-682-local-friendly-devel…

e9cee2e

…opment-and-commonjs Conflicts: scripts/sdk/build.sh

Explain what is being deleted by the strange syntax

f1e07b6

See #693 (comment)

Remove extra space

ce289ba

Update Vite to avoid flakey errors in our PostCSS plugins

d053d43

Fix #722 Updating Vite to includes fixes from vitejs/vite#7822 -> vitejs/vite#7827

MadLittleMods commented May 5, 2022

View reviewed changes

MadLittleMods requested a review from bwindels May 5, 2022 20:08

MadLittleMods added 3 commits May 5, 2022 17:50

Merge branch 'master' into madlittlemods/686-682-local-friendly-devel…

75098b4

…opment-and-commonjs

Add some comments

e54482e

Merge branch 'master' into madlittlemods/686-682-local-friendly-devel…

639358b

…opment-and-commonjs Conflicts: scripts/sdk/base-manifest.json

bwindels reviewed May 16, 2022

View reviewed changes

bwindels added this to the er21mat22kenavo milestone May 17, 2022

Clean up index.html in the right spot

b725269

MadLittleMods commented May 18, 2022

View reviewed changes

MadLittleMods requested a review from bwindels May 18, 2022 05:25

bwindels approved these changes May 30, 2022

View reviewed changes

Merge branch 'master' into madlittlemods/686-682-local-friendly-devel…

1b2a6b5

…opment-and-commonjs

bwindels merged commit a49d7ea into master May 30, 2022

MadLittleMods added a commit that referenced this pull request May 31, 2022

Import assets from the assets/ directory

38c3774

> Will be easier towards the future when adding more assets. Probably best to keep style.css for now for backwards compat though. > > *-- #693 (comment)

MadLittleMods mentioned this pull request May 31, 2022

Import SDK assets from the assets/ directory #746

Merged

MadLittleMods mentioned this pull request Jun 6, 2022

Update to work with latest hydrogen-view-sdk@0.0.12 matrix-org/matrix-viewer#10

Merged

Conversation

MadLittleMods commented Feb 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dev notes

Use the latest Vite from GitHub source (before release)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Feb 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Feb 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bwindels left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwindels left a comment

Choose a reason for hiding this comment

Uh oh!

MadLittleMods May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwindels left a comment

Choose a reason for hiding this comment

Uh oh!

MadLittleMods commented May 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MadLittleMods commented Feb 26, 2022 •

edited

Loading

MadLittleMods Feb 26, 2022 •

edited

Loading

MadLittleMods Feb 26, 2022 •

edited

Loading

MadLittleMods May 5, 2022 •

edited

Loading

MadLittleMods May 18, 2022 •

edited

Loading