Remove dependency on screenfull - fix issue#107

[BePo65]

In some scenarios (e.g. commonjs environments) there exist problems with the dependency on the screenfull package. This package is a second export of the leaflet.fullscreen package, but this seems not to work in some configurations.

As screenfull is only a thin wrapper around the fullscreen api available in all modern browsers, this pr integrates the required calls to the fullscreen api into the leaflet.fullscreen package as internal methods.

As all major browsers support the fullscreen api since more than 4 years and since IE is no longer supported by microsoft, I dropped the support of older browsers / IE (but this could be changed easily).
The only exception is the safari browser on apple devices that only support the complete api since march 2023. Here older versions are supported too.

I tested this pr in the demo page included in the package and in the commonjs demo (using hugo) from @taviowong. To enable more tests, I set the version in the package.json file to v 3.0.0-rc.0, so that this pr could be published as a prerelease (using npm publish --tag next).

Perhaps some of the other users (@barbalex or @ricfio) could test this release candidate too (with npm i leaflet.fullscreen@next) and give some feedback. I would appreciate this very much.

[ricfio]

Perhaps some of the other users (@barbalex or @ricfio) could test this release candidate too (with npm i leaflet.fullscreen@next) and give some feedback. I would appreciate this very much.

Hi @BePo65, I tried to do this on my project:

npm r screenfull
npm i github:BePo65/leaflet.fullscreen#b1e18f7c
4814cae8a857c8288b8ec56763fb57c9

but the result was:

vite

✘ [ERROR] Could not resolve "screenfull"

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

  You can mark the path "screenfull" as external to exclude it from the bundle, which will remove
  this error. You can also surround this "require" call with a try/catch block to handle this
  failure at run-time instead of bundle-time.

/home/ricfio/cedat85/projects/d4d/portal-NEW/node_modules/esbuild/lib/main.js:1649
  let error = new Error(`${text}${summary}`);
              ^

Error: Build failed with 1 error:
node_modules/react-leaflet-fullscreen/node_modules/leaflet.fullscreen/Control.FullScreen.js:196:55: ERROR: Could not resolve "screenfull"
    at failureErrorWithLog (/home/ricfio/cedat85/projects/d4d/portal-NEW/node_modules/esbuild/lib/main.js:1649:15)
    at /home/ricfio/cedat85/projects/d4d/portal-NEW/node_modules/esbuild/lib/main.js:1058:25
    at /home/ricfio/cedat85/projects/d4d/portal-NEW/node_modules/esbuild/lib/main.js:1525:9
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  errors: [
    {
      detail: undefined,
      id: '',
      location: {
        column: 55,
        file: 'node_modules/react-leaflet-fullscreen/node_modules/leaflet.fullscreen/Control.FullScreen.js',
        length: 12,
        line: 196,
        lineText: "\t\tmodule.exports = factory(require('leaflet'), require('screenfull'));",
        namespace: '',
        suggestion: ''
      },
      notes: [
        {
          location: null,
          text: 'You can mark the path "screenfull" as external to exclude it from the bundle, which will remove this error. You can also surround this "require" call with a try/catch block to handle this failure at run-time instead of bundle-time.'
        }
      ],
      pluginName: '',
      text: 'Could not resolve "screenfull"'
    }
  ],
  warnings: []
}

Node.js v18.17.1

[ricfio]

so, i have solved in this way:

npm i screenfull

then I'm come back to official leaflet:

npm r leaflet.fullscreen # to remove the pr version
npm i leaflet.fullscreen # to re-install the official 2.4.0 version

I hope this can help you.

[BePo65]

@ricfio I see that you are using react-leaflet-fullscreen that is a wrapper around leaflet.fullscreen. As react-leaflet-fullscreen connot yet be updated to the updated leaflet.fullscreen you have to do some more steps to test the updated functionality (all used directories must already exist after step 2):

1. npm r screenfull
2. npm i github:BePo65/leaflet.fullscreen#b1e18f7c4814cae8a857c8288b8ec56763fb57c9
3. copy the file Control.FullScreen.js containing the new version from 'node_modules/leaflet.fullscreen' to the node_modules used by 'react-leaflet-fullscreen' in the directory 'node_modules/react-leaflet-fullscreen/node_modules/leaflet.fullscreen'

now running vite should use the updated version.

To undo this change you have to reinstall 'react-leaflet-fullscreen' and 'screenfull'..

I know this is a lot of work, but it would help me a lot, to make sure that the modifications run in your environment.

[ricfio]

@BePo65 I have replaced node_modules/react-leaflet-fullscreen/node_modules/leaflet.fullscreen/Control.FullScreen.js with node_modules/leaflet.fullscreen/Control.FullScreen.js (from github:BePo65/leaflet.fullscreen#b1e18f7c4814cae8a857c8288b8ec56763fb57c9)

It's works ! ... but at this point, after the pr closing, will be need to update react-leaflet-fullscreen: https://github.com/krvital/react-leaflet-fullscreen

[BePo65]

I will create an issue in 'react-leaflet-fullscreen' as soon, as this pr is accepted and published.

[BePo65]

As the changes in this pr seems to solve the problem, I think that we can skip the release of a prerelease version; therefore the version info in the packag.json must be changed before publishing a version containing thhis pr.

[brunob]

Version 3.0.0 published, thx again @BePo65

Since you are now the main contributor of this package i would like to suggest to transfer the ownership of this repo to @Leaflet org. This could allow more people to collaborate on this, and i could help to handle issue and request on this (since i'm not really coding on it since a long time). Any thoughts ?

[BePo65]

Perhaps it would help the leaflet guys to accept such a transfer, if we added some development "helpers":

1. add 'uglify-js' to create a minified version to the repository (or how does the minified version come to the cdn?)
2. perhaps add a script to add the version number to the head of the javascript file

If you agree, I would create a pr for this.

[brunob]

1. add 'uglify-js' to create a minified version to the repository (or how does the minified version come to the cdn?)

why not this could be done like https://github.com/Leaflet/Leaflet.heat/blob/gh-pages/package.json#L26 but minified version on https://cdnjs.com/libraries/leaflet.fullscreen seems to be automatically generated

2. add eslint using the configuration from the leaflet project to ensure consistent coding styles

I can't find any guideline way on this side, maybe we should do like this https://github.com/Leaflet/Leaflet.draw/blob/develop/package.json#L7 & https://github.com/Leaflet/Leaflet.draw/blob/develop/package.json#L67

3. perhaps add a script to add the version number to the head of the javascript file

Not sure bout that, but if you think it's use full, gogogo :)

If you agree, I would create a pr for this.

Nice !

[brunob]

PS : do you think this mention to screenfull in the readme is still revelant ? https://github.com/brunob/leaflet.fullscreen/blob/master/README.md?plain=1#L23

[BePo65]

Regarding the 'ps': looks like I missed that point. Will fix it in a mini pr.

Regarding the other points:

if I understand your answer, you do not have to publish your package ti cdnjs - somehow they fetch it from github (I did not find an answer to this question on the web), The idea behind point 3 was to make sure that I can find out, what version I'm using, But cdnjs adds the version to the path; therefore I think we don't need neither point 1 nor point 3.

And regarding point 2: the idea was to take (some) rules from the .eslintrc,js of the leaflet repository.

[BePo65]

One more question: in the first line of 'Control.FullScreen.js' there is a !_map after the start of the comment.

As I didn't find any information about that "token" on the web: can you tell me,what that is about? Eslint complains about that sequence, when I use the settings from the leaflet repository.

[brunob]

And regarding point 2: the idea was to take (some) rules from the .eslintrc,js of the leaflet repository.

Perfect.

One more question: in the first line of 'Control.FullScreen.js' there is a !_map after the start of the comment.

This was introduced with 768329bc287dd3a976e56d48e816176de5ffb749 ^^ Is it a typo ? Or did you get it from another leaflet plugin ?

[BePo65]

Dammed, git never forgets anything 😃