-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add Teapot example that uses the new ES module bundle with importmap #5645
Conversation
4509673
to
6b02a87
Compare
{ | ||
"imports": { | ||
"aframe": "../../../dist/aframe-master.module.min.js", | ||
"three": "https://cdn.jsdelivr.net/npm/[email protected]/build/three.module.js", |
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.
Problem is that will have to update this in the future since it’s locked to r172. Is it possible to have an example without a hardcoded version?
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.
It could be an option to tag the super-three versions with the A-Frame version they are used in, so you'd be able to use [email protected]
. For master we could consider pointing it to node_modules
instead of using a CDN? That way when checking out older revisions, running npm install
ensures that the right super-three version is referenced.
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.
yeah. pointing to node_modules sounds fine to me. trying to avoid small things that will eventually break and hard to keep track of over time.
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.
Did you see my changes to the release script? There is no need to update manually, the script will update the super-three version in examples and docs similar to the aframe version.
But indeed for master that should reference the current version in node_modules. I just added an additional directory path to the webpack dev server config, and the preghpages script rewrite the url with the cdn.
examples/index.html
Outdated
@@ -157,6 +157,7 @@ <h2>Examples</h2> | |||
<li><a href="boilerplate/3d-model/">3D Model (glTF)</a></li> | |||
<li><a href="mixed-reality/anchor/">Anchor (Mixed Reality)</a></li> | |||
<li><a href="mixed-reality/real-world-meshing/">Real World Meshing (Mixed Reality)</a></li> | |||
<li><a href="boilerplate/teapot/">Teapot (use importmap and import from three/addons)</a></li> |
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.
I think it might be better to rename the example to importmap. The way I see it, it's an example of using importmap using a Teapot, instead of an example of a Teapot using importmaps.
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.
yeah. boilerplate/importmap or boilerplate/modules. not sure
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.
Okay, I renamed to boilerplate/importmap.
I did the changes. There is no manual changes needed to update url or version for super-three in development with webpack or when the docs are released, all is done via the preghpages and prerelease scripts. |
Thank you! |
Description:
Add Teapot example that uses the new ES module bundle with importmap.
Changes proposed: