apache / incubator-annotator

Apache Annotator provides annotation enabling code for browsers, servers, and humans.
https://annotator.apache.org/
Apache License 2.0
219 stars 44 forks source link

Can't use after installing via npm (without webpack or similar) #113

Closed DellCliff closed 2 years ago

DellCliff commented 3 years ago

The project can't be used in the browser after installing it via npm because the files reference

import _Object$keys from "@babel/runtime-corejs3/core-js/object/keys"; import _sliceInstanceProperty from "@babel/runtime-corejs3/core-js/instance/slice"; import _Array$from from "@babel/runtime-corejs3/core-js/array/from"; import _Symbol from "@babel/runtime-corejs3/core-js/symbol"; import _getIteratorMethod from "@babel/runtime-corejs3/core-js/get-iterator-method"; import _getIterator from "@babel/runtime-corejs3/core-js/get-iterator";

causing

Uncaught TypeError: Error resolving module specifier “@babel/runtime-corejs3/core-js/object/keys”. Relative module specifiers must start with “./”, “../” or “/”.

Solution:

Publish truly pre-compiled artifacts (e.g. annotator.min.js) on npm that can be used in the browser without webpack.

Treora commented 3 years ago

Correct, the package is currently not pre-bundled for use in the browser, as the getting started page says: “Currently we only support installation through NPM packages. You will need to use a bundler (such as webpack) to use the modules in a web browser.”

If there is demand for it, and apparently there is, this would be a nice thing to provide, either in the npm packages, or we could distribute bundled/browser-compatible packages separately.

@tilgovi do you have ideas about this? What approaches we could take?

DellCliff commented 3 years ago

It could run in the browser, if imports like these (ES6 imports instead of node imports) were generated:

import _Object$keys from "../@babel/runtime-corejs3/core-js/object/keys";
import _sliceInstanceProperty from "../@babel/runtime-corejs3/core-js/instance/slice";
import _Array$from from "../@babel/runtime-corejs3/core-js/array/from";
import _Symbol from "../@babel/runtime-corejs3/core-js/symbol";
import _getIteratorMethod from "../@babel/runtime-corejs3/core-js/get-iterator-method";
import _getIterator from "../@babel/runtime-corejs3/core-js/get-iterator";

../ here would walk up to the node_modules folder.

This presumes that those babel imports also generate ES6 style module files.

DellCliff commented 3 years ago

Maybe there is also a way to tell TypeScript (or which ever compiler is responsible) to generate ES6 module imports instead of node imports.

tilgovi commented 3 years ago

We are generating ES6 imports/exports. We don't actually ship CommonJS at all right now. But npm package imports are not valid in the browser because the browser only allows imports from URLs (relative or absolute).

I would have expected some of the CDNs that support bundling to work, like importing import dom from 'https://cdn.skypack.dev/@apache-annotator/dom@dev'. It looks like the optimal-select package declares a its src/index.js as its module entrypoint, though, but doesn't ship the src directory in its npm package, so that breaks.

We can try to fix that, and we should absolutely consider more ways to package and ship, so we appreciate hearing what would be helpful and even more appreciate any help updating the packaging!

tilgovi commented 3 years ago

Looks like that problem has been reported here: https://github.com/autarc/optimal-select/issues/70

We could drop the "describe" support for Element -> CSS selector conversion, or try to find another package for that, or roll our own basic one. That package hasn't been updated since 2017.

tilgovi commented 3 years ago

I propose we switch to @medv/finder. It is actively maintained (for now), written in TypeScript, seems properly packaged, has no dependencies, and has an MIT license.

We could still consider shipping browser bundles, but at least this change should make it possible to load a browser bundle through a CDN like snowpack.

tilgovi commented 3 years ago

There's a project that compares some of these CSS selector generation libraries, and it seems to find this one favorable, too: https://github.com/fczbkk/css-selector-generator-benchmark

I checked out the code and it also looks like it support passing a document in explicitly—so it doesn't have any dependencies on globals—and passing in a root element—great for generating refining selectors.

Treora commented 3 years ago

Regarding replacing the optimal-select dependency, I described my earlier research in commit message c8ef340e34b1:

I tried a few css selector generators, listed here:
<https://github.com/fczbkk/css-selector-generator-benchmark>

- css-selector-generator failed when a root (= scope) is passed; see
  issue <https://github.com/fczbkk/css-selector-generator/issues/65>.

- using @mdev/finder instead gave syntax errors due to ‘export’ token.
  (perhaps because we don’t transpile dependencies; worth considering?)

- optimal-select seemed to work; whatever works is good enough for now.
tilgovi commented 3 years ago

Ah, I see. It shouldn't be a problem that @mdef/finder is ESM-only, since we only support versions of Node that have stable ESM support. However, it looks like the landscape of TypeScript -> ESM -> Mocha is very fresh right now.

It's easy to stop transforming the module syntax by changing our Babel configuration, but Mocha still won't recognize the .ts files as ESM. The @babel/register package doesn't support ESM, because ESM loader support in Node.js is still experimental. There are packages like babel-register-esm and ts-node that try to use this, and we would be able to use this with the loader option in newer versions of Mocha, but this all seems like a bleeding edge minefield right now.

Alternatively, we might compile our dependencies, or at least that one, which is a change we could make in our Babel config.

tilgovi commented 3 years ago

Actually, even compiling the dependencies won't work, because @mdev/finder specifies "type": "module" in its package.json, which means Node will try to load it as ESM anyway, even if it is compiled to no longer uses the export syntax.

I'll continue to work on this.