skypackjs / skypack-cdn

An issue tracker for the CDN
107 stars 5 forks source link

three@v0.137.0 deep imports don't work anymore #261

Closed marcofugaro closed 2 years ago

marcofugaro commented 2 years ago

The import { ... } from 'three' import is not resolved anymore since last release, while in last release was working correctly.

For context, this is the import I am talking about: https://github.com/mrdoob/three.js/blob/438ac79c2705af9f1d3fb7fd34345564d23f1644/examples/jsm/controls/OrbitControls.js#L1-L9

It should be automatically resolved to three (like it was in last release).

Looking at the URLs, the only difference I see is raw in place of import/unoptimized, what does it mean?

Current release

https://cdn.skypack.dev/three@0.137.0/examples/jsm/controls/OrbitControls.js

Last release

https://cdn.skypack.dev/three@0.136.0/examples/jsm/controls/OrbitControls.js


For additional context, these fields of package.json have changed in this release, may be related.

image

FredKSchott commented 2 years ago

It looks like Three.js now ships an export map. See the "exports" entry in the new package.json:

This is three.js telling you what you can and cannot import out of the three.js package. If you were to import three/examples/jsm/controls/OrbitControls.js inside of Node.js, it would also now fail in v0.137.0.

Instead of failing, we just default to the raw JS file, which can work in some cases but will not resolve imports the way that you need them here.

You can reach out to the three.js team to get them to add examples to their export map. Otherwise, you'll need to stay pinned on v0.136.0, or pull that example into your local project.

FredKSchott commented 2 years ago

Closing since Skypack is working as expected

marcofugaro commented 2 years ago

You can reach out to the three.js team

I am the one who made those changes on three.js.

If you were to import three/examples/jsm/controls/OrbitControls.js inside of Node.js, it would also now fail in v0.137.0.

Not true, this works correctly:

import * as THREE from 'three'
import { OrbitControls } from 'three/examples/jsm/controls/OrbitControls';

console.log(OrbitControls)

You can test it by yourself by downloading this zip: three-test.zip Then run:

As you can see from the screenshot I posted, the examples are included in the exports entry.

marcofugaro commented 2 years ago

May the the extra .js extension be the cause of this issue?

"./examples/jsm/*": "./examples/jsm/*.js",

Hmm we probably want

"./examples/jsm/*": "./examples/jsm/*",

would this work?

FredKSchott commented 2 years ago

Ah, interesting! Apologies for dismissing this then without digging deeper.

Our goal is to match Node.js behavior here. If you change the Node.js import to three/examples/jsm/controls/OrbitControls.js (with the file extension, to match what you're doing with Skypack), does it still work in Node.js?

marcofugaro commented 2 years ago

It currently fails because of the extra .js extension

image

Is this why skypack is failing?

marcofugaro commented 2 years ago

@FredKSchott we released a patch version, any idea why it is still raw?

https://cdn.skypack.dev/three@0.137.1/examples/jsm/controls/OrbitControls.js

This works correctly in node:

import { OrbitControls } from 'three/examples/jsm/controls/OrbitControls.js';

console.log(OrbitControls)
ycw commented 2 years ago

@FredKSchott @marcofugaro It seems skypack honors this package.json according to this meta, does skypack handle "weird"(missing fields) package.json the same way of nodejs? I tried to import a random file from './src/*' ( src/ doesn't contain a package.json) skypack sent back a non raw as expected.

FredKSchott commented 2 years ago

Ah, that could be it. Skypack has some handling for sub-package package.json mainfests, which is probably why this was working before. Maybe now the fact that there are subpackage package.jsons manifest AND an "exports" entry in the main package.json is breaking Skypack. In that case, this could be a bug in Skypack that we'd need to address (might take some time).

As @ycw mentioned this doesn't seem to be a problem for the src/ directory, only examples/

FYI node.js has this to say in their docs, about adding an "exports" map:

https://nodejs.org/api/packages.html#package-entry-points Warning: Introducing the "exports" field prevents consumers of a package from using any entry points that are not defined, including the package.json (e.g. require('your-package/package.json'). This will likely be a breaking change.

To make the introduction of "exports" non-breaking, ensure that every previously supported entry point is exported. It is best to explicitly specify entry points so that the package’s public API is well-defined. For example, a project that previous exported main, lib, feature, and the package.json could use the following package.exports:

FredKSchott commented 2 years ago

I definitely don't want to break existing users, but I feel like I should ask: Are examples/ meant to be imported out of the three.js package? I would assume examples are for learning and copying, and not necessarily meant to be imported directly by users in a website.

mrdoob commented 2 years ago

I definitely don't want to break existing users, but I feel like I should ask: Are examples/ meant to be imported out of the three.js package? I would assume examples are for learning and copying, and not necessarily meant to be imported directly by users in a website.

Yep, you're right. Unfortunately that boat sailed a long time ago.

marcofugaro commented 2 years ago

Maybe now the fact that there are subpackage package.jsons manifest AND an "exports" entry in the main package.json is breaking Skypack.

@FredKSchott we could remove the subpackage package.json now that it is unnecessary (the main package has type: "module"), if that's gonna make it work with Skypack:

https://github.com/mrdoob/three.js/blob/dev/examples/jsm/package.json

marcofugaro commented 2 years ago

Could you reopen this issue in the meantime?

marcofugaro commented 2 years ago

Did a quick test by removing the subpackage package.json you guys were mentioning, and it has no effect: https://cdn.skypack.dev/three-secret-test-f89ahsf9a@v0.137.4/examples/jsm/controls/OrbitControls.js

@FredKSchott If you could find a way to make skypack work with the exports field it would be great 🙏

marcofugaro commented 2 years ago

May this be related to #249?

ycw commented 2 years ago

@FredKSchott seems skypack breaks nodejs packages: subpath-patterns rules:

All instances of * on the right hand side will then be replaced with this value, including if it contains any / separators.

Current skypack behavior is incompatilble with this, for example:

Given package.json by @marcofugaro :

  "exports": {
    "./src/*": "./src/*",
Tests: import result
src/utils.js serve as mode=imports/optimized/..
src/math/Euler.js serve as mode=raw/.. 🤔

In fact, meta packageExports shows that skypack doesn't handle subpath-patterns recursively.

@marcofugaro try .. exports all subpaths down to leaf,

  "exports": {
    "./examples/jsm/controls/*", "./examples/jsm/controls/*"
marcofugaro commented 2 years ago

Thanks @ycw! Did a quick test and that seems to be the issue!

Opened #263 since this issue seems to remain closed.