Viglino / ol-ext

Cool extensions for Openlayers (ol) - animated clusters, CSS popup, Font Awesome symbol renderer, charts for statistical map (pie/bar), layer switcher, wikipedia layer, animations, canvas filters.
https://viglino.github.io/ol-ext/
Other
1.22k stars 464 forks source link

Svelte and CJS vs ES module issue #863

Closed SimonSchneider closed 1 year ago

SimonSchneider commented 1 year ago

Trying to import ol-ext in a svelteKit project gives the following error:

ol-ext doesn't appear to be written in CJS, but also doesn't appear to be a valid ES module (i.e. it doesn't have "type": "module" or an .mjs extension for the entry point). Please contact the package author to fix.

Given that OpenLayers defines "type": "module" (link) maybe it'd be reasonable to ol-ext to do the same?

Viglino commented 1 year ago

Added to the package.json. Look at https://github.com/Siedlerchr/types-ol-ext to have typescript definition.

SimonSchneider commented 1 year ago

That was fast, thank you very much!

Will check if the typescript definitions require "type": "module", thanks!

Any idea on when a new release will be available?

Viglino commented 1 year ago

I've published a 4.0.1. Tell me if it runs.

isti115 commented 1 year ago

This breaks webpack for me with the following error:

ERROR in ./node_modules/ol-ext/layer/GeoImage.js 5:0-43 Module not found: Error: Can't resolve 'ol/layer/Image' in '/home/isti/prog/collmot/skybrush-io/live/node_modules/ol-ext/layer' Did you mean 'Image.js'? BREAKING CHANGE: The request 'ol/layer/Image' failed to resolve only because it was resolved as fully specified (probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '.mjs' file, or a '.js' file where the package.json contains '"type": "module"'). The extension in the request is mandatory for it to be fully specified. Add the extension to the request.

I guess that if you want to keep the "type": "module" option moving forward, then file extensions will have to be added to all imports.

Viglino commented 1 year ago

See #868

SimonSchneider commented 1 year ago

Sorry for the delay, I've tested it and 4.0.1 has been working great

Viglino commented 1 year ago

@SimonSchneider

Sorry for the delay, I've tested it and 4.0.1 has been working great

It seems that extension is required when using "type": "module"

SimonSchneider commented 1 year ago

I'm unsure, when I tested a local branch I only had issues with the gulpfile needing to be changed to be .mjs the rest seemed to work. For my project the addition of "type": "module" which you released resolved the issue.

isti115 commented 1 year ago

I have quickly grepped through the source and didn't see any reason not to simply just add the .js extension to all the import declarations, so I executed s/(^import .*["'])(.*)(["'])/$1$2.js$3/g in the repo and created a pull request from it (#869), so in case you agree with this approach, @Viglino, you can just merge it right away.

ps.: It might be worth getting rid of the double quoted imports and turning them into single quotes for consistency, but I didn't want to make unnecessary side effects. Also, some imports had semicolons at the end and some didn't.

Viglino commented 1 year ago

@Isti115 That's what I planned to do! you beat me 🚀

Viglino commented 1 year ago

ol-ext@4.0.2 is out

Siedlerchr commented 1 year ago

For what it worth, in the types-ol-ext repo I used the following addtion to the webpack to compile the examples:


  {
        test: /\.m?js/,
        type: "javascript/auto",
      },
      {
        test: /\.m?js/,
        resolve: {
          fullySpecified: false,
        },
      },
    ],
Siedlerchr commented 1 year ago

@Viglino I ust noticed that there is a duplicate .js extension: in Overlay/Tooltip

import {getArea as ol_sphere_getArea} from 'ol/sphere.js.js';
import {getLength as ol_sphere_getLength} from 'ol/sphere.js.js';
Viglino commented 1 year ago

@Siedlerchr fixed duplicated extension (there was some more)

isti115 commented 1 year ago

Ouch, sorry, that was my bad, I should've checked for already present extensions before running the RegEx replacement, but for some reason I assumed that all imports were extensionless so far.