Dependency on screenfull@5.2.0 should be specified

[taviowong]

It is not clear that when used in an npm environment, screenfull should be installed beforehand.

Also, the last version of screenfull that can be used with leaflet.fullscreen (for now) is 5.2.0 as later versions cannot be require()'d any more as noted on their page. I spent some time debugging why the fullscreen api was not working until I found the default export was not expected by leaflet.fullscreen.

Maybe you could add a note on the dependency to the readme file? Or better yet, add the dependency to package.json so screenfull will be installed automatically via npm? Thanks.

[brunob]

related to #81 ?

[taviowong]

Not exactly.

If screenfull is not installed and you import 'leaflet.fullscreen', you will get the "screenfull is not defined" error.

If you run npm install screenfull, version 6.0.2 is installed and you no longer get the "screenfull is not defined" error. However, toggling full screen will silently fail and fallback to pseudo-fullscreen mode because screenfull is not loaded properly in the current leaflet.fullscreen.

Without modifying any code in this project, the simplest solution is to pin screenfull to version 5.2.0, which is the last version that works here, either manually or via package.json. Hence the suggestion to add a note to readme to save newcomers some debugging time. And is there any reason why screenfull is not added as a dependency to package.json?

[brunob]

And is there any reason why screenfull is not added as a dependency to package.json?

No strict reason, just that i don't use this script in an npm environment :p

@BePo65 your thought on this ?

[taviowong]

On second thoughts, you might affect the 26 dependent projects if you update the dependency now.

Maybe just ask people to install screenfull@5.2.0 manually if they intend to use it in an npm environment? :P

[BePo65]

the current version of 'leaflet.fullscreen' does not require 'screenfull' as a clone of this package is an integral part of 'leaflet.fullscreen'. I tested this today again (just in case :-) and in a pure html environment I get no error message.

You say, you are using an 'npm' environment.
Can you give us some more information e.g. like what framework you are using (or better yet a small demo on something like stackblitz), so I can reproduce the effect.

[taviowong]

Not sure why I can't get stackblitz to run fullscreen on my laptop, but here's the link: https://stackblitz.com/edit/js-k6h3bh?file=index.js

You can compare the browser console with screenfull@5.2.0 and screenfull@6.0.2. With 5.2.0 you can see that screenfull is using the fullscreen api (which is blocked because of iframe policy), but with 6.0.2 there is no error, which means the fullscreen api is not invoked at all.

[BePo65]

Sorry, but I can only partly reproduce the issue.
I tried you stackblitz example and it shows the effect, when the dependency "screenfull" is removed. But I don't know, what kind of framework this exmple uses - if I download the project, but there seems to be missing something to get a running program from it.

So I made a look-alike in angular and it shows the same warning / error in stackblitz.
But when you download the project, run npm install and then npm start in the directory of the project, everything runs fine: you get the fullscreen button and clicking it has the desired effect.

To me it looks like stackblitz has "problems" with the resolution of the amd package 'leaflet.fullscreen' described (and solved) in issue #83.

Can you give me more details about your framework (angular, react, or whatever this might be)?

[taviowong]

I'm using Hugo to build a static site. Hugo uses ESBuild that can bundle npm code in browser.

I made a really simple demo at https://github.com/taviowong/hugo-leaflet-demo and you can checkout the code and build for yourself.

Assuming you have npm and hugo installed, run npm i && hugo server and go to http://localhost:1313/. The fullscreen button works fine because I'm using screenfull@5.2.0 here.

Now open another terminal and go to the same folder and run npm i screenfull@latest, refresh the browser and click the fullscreen button again. This time it falls back to the pseudo fullscreen implementation.

The JS source code is in assets/map.js and the output file is not minified. leaflet.fullscreen expects to find the screenfull object under the key "screenfull", but in the latest version of screenfull the object is wrapped inside a default export and I guess that's why it's not working.

[BePo65]

Thanks for the demo; I will try to track the problem down - will take a few days :-)

[barbalex]

Happens to me to. And it is completely logical it does:

X [ERROR] Could not resolve "screenfull"

    node_modules/leaflet.fullscreen/Control.FullScreen.js:196:55:
      196 │     module.exports = factory(require('leaflet'), require('screenfull'));

as screenfull is required but not declared as dependency.

[ricfio]

Happens to me to. And it is completely logical it does:

X [ERROR] Could not resolve "screenfull"

    node_modules/leaflet.fullscreen/Control.FullScreen.js:196:55:
      196 │     module.exports = factory(require('leaflet'), require('screenfull'));

as screenfull is required but not declared as dependency.

Hi, I solved the build error adding 'resolve.alias.screenfull' in my vite.config.ts. ... but the fullscreen does not working... I'll try with npm i screenfull@5.2.0

import { defineConfig } from 'vite'
import react from '@vitejs/plugin-react'

// https://vitejs.dev/config/
export default defineConfig({
  plugins: [react()],
  build: {
    target: 'esnext'
  },
  resolve: {
    alias: {
      'screenfull': 'node_modules/leaflet.fullscreen/Control.FullScreen.js',
    },
  },  
})

[ricfio]

Happens to me to. And it is completely logical it does:

X [ERROR] Could not resolve "screenfull"

    node_modules/leaflet.fullscreen/Control.FullScreen.js:196:55:
      196 │     module.exports = factory(require('leaflet'), require('screenfull'));

as screenfull is required but not declared as dependency.

Hi, I solved the build error adding 'resolve.alias.screenfull' in my vite.config.ts. ... but the fullscreen does not working... I'll try with npm i screenfull@5.2.0

import { defineConfig } from 'vite'
import react from '@vitejs/plugin-react'

// https://vitejs.dev/config/
export default defineConfig({
  plugins: [react()],
  build: {
    target: 'esnext'
  },
  resolve: {
    alias: {
      'screenfull': 'node_modules/leaflet.fullscreen/Control.FullScreen.js',
    },
  },  
})

Yes! It's now working...

I removed this from my vite.config.ts:

...
  resolve: {
    alias: {
      'screenfull': 'node_modules/leaflet.fullscreen/Control.FullScreen.js',
    },
  }, 
...

After I installed screenfull@5.2.0: npm i screenfull@5.2.0

... and now, finally, also the fullscreen API works fine!

PS. @brunob these steps needs if you want use leaflet.fullscreen with npm, react, etc... I have installed also react-leaflet-fullscreen. These are some dependencies in my package.json:

  "dependencies": {
...
    "leaflet": "^1.9.4",
    "leaflet.fullscreen": "^2.4.0",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "react-leaflet": "^4.2.1",
    "react-leaflet-fullscreen": "^4.1.0",
    "screenfull": "^5.2.0"
  },

[BePo65]

Is it really already october 2023? Somehow this issue must have slipped my attention - I am so sorry.

Anyhow: I thought that e.g. esbuild should be able to use this umd package. "leaflet.fullscreen" should publish "leafletFullScreen" and "screenfull" as global packages (and in a pure browser environment it does).

So I have to analyze, what the packagers do with the second export of leaflet.fullscreen. I'll be back soon.

[BePo65]

So here are my results:

1. there is an error in the umd definition for leaflet.fullscreen; inadvertently I forgot to add a property to the module.exports and therefore the export for screenfull (from the first part of the file) was overwritten. So there is no commonjs definition for the screenfull 'package' included in leaflet.fullscreen.

2. By fixing the first problem we come to the real problem: in the umd definition for leaflet.fullscreen we have to define screenfull as a requirement for leaflet.fullscreen. But there is no default export for screenfull in this file - only a deeper nested export.
   By adding a dependency for screenfull to the package.json this problem was "fixed", but this is not the primary intension of the author of this package to add screenfull to the Control.FullScreen.js file.

The idea behind adding screenfull to the Control.FullScreen.js file was that only 1 file has to be included in a web page to get the fullscreen button for leaflet. In a pure html web page this even works, because in this case Control.FullScreen.js sets the global correspondingly.

But I didn't find a way to reproduce this behavior in a commonjs environment (thanks to @taviowong for the demo).

As screenfull is only a thin layer around the official fullscreen api and as all browser support this api since 2019 (ok, safary only since the beginning of this year), I would propose to integrate the calls to the fullscreen api directly into lealet.fullscreen without using screenfull ata all (neither as an external reference nor as an integral part of Control.FullScreen.js).

@brunob: I could prepare a pr so that you can find out, if you like the idea 😄 . Just give a go if you agree.

[brunob]

As screenfull is only a thin layer around the official fullscreen api and as all browser support this api since 2019 (ok, safary only since the beginning of this year), I would propose to integrate the calls to the fullscreen api directly into lealet.fullscreen without using screenfull ata all (neither as an external reference nor as an integral part of Control.FullScreen.js).

I've chosen to use screenfull in order to get rid of all questions/issues about "this doesn't work on XXX" (put any browser and iProduct in place of XXX ^^). Support of the fullscreen API is 95% now https://caniuse.com/fullscreen so maybe we can do the switch you propose, and bump a major version "pour marquer le coup".

Thx again, and gogogo !

[BePo65]

Gimme a few days 😄

[brunob]

Absolutely no hurry @BePo65 !

[brunob]

Closed with #113