Built file in master requires fs

[trygveaa]

mapbox-gl-js version: 1.8.1
mapbox-gl-draw version: master

Steps to Trigger Behavior

1. Clone this repo and run yarn && yarn build-min
2. Try to import dist/mapbox-gl-draw.js in a project

Expected Behavior

It should work, without depending on fs.

Actual Behavior

I can't use it because it depends on fs. That is, I'm getting:

ERROR in ./node_modules/@mapbox/mapbox-gl-draw/dist/mapbox-gl-draw.js
Module not found: Error: Can't resolve 'fs' in '/home/trygve/dev/my-project/front/node_modules/@mapbox/mapbox-gl-draw/dist'

If I try to do the same after checking out v1.1.2 it works fine.

[Ecafracs]

I'm also encountering this issue.
It looks like mapbox-gl-draw depends on @mapbox/geojsonhint, which depends on zaach/jsonlint-lines, which is referring to fs.
@mapbox/geojsonhint is apparently "on pause" as of a year ago.
zaach/jsonlint-line was abandoned 4 years ago, and it looks like mapbox/jsonlint is a fork of a fork of it.

Can somebody who understand what these packages do figure out how to replace these dependencies to not rely on dead code?

[brendanmc6]

A fix would be much appreciated! @trygveaa would that PR you've linked fix this issue as well? Or would we need to wait for a downstream fix here?

As others have said, this seems to fix it for me:
import MapboxDraw from '@mapbox/mapbox-gl-draw/dist/mapbox-gl-draw.js';

Here are existing issues I have found:
#850
#844
#626

[trygveaa]

A fix would be much appreciated! @trygveaa would that PR you've linked fix this issue as well? Or would we need to wait for a downstream fix here?

Yes, #977 fixes this issue. However I've written some comments for discussion in it, because I don't think bundling all the dependencies in the single js file is a good idea. Though, that already happens, so there are no downsides of #977 over the current situation.

As others have said, this seems to fix it for me:
import MapboxDraw from '@mapbox/mapbox-gl-draw/dist/mapbox-gl-draw.js';

That works because the issue I'm describing in here doesn't exist in version v1.1.2, which is what is currently on npm. However for master, i.e. the unreleased version, this solution does not work.

Here are existing issues I have found:
#850
#844
#626

To be clear, even though these are related, the issue I'm describing here is not the same as those issues. Those issues is about the main entry point for the package requiring fs and path. That can be worked around by importing the dist file instead as you mentioned. However, this issue is about that even the dist file requires fs and path, so this workaround does not work anymore.

PR #976 fixes what those issues describe, so you don't have to specify the dist file (but #977 is also required for that to work).

[georgir]

Now v1.2.0 is the current on npm and this module has become unusable by default because of this issue. I don't get how could it get out of beta and published like that.
edit: ok, the usage via old-fashioned script tag and no module system still works, so it's not as bad as I thought at first glance

[dougturnkey]

Hi! I am a mapbox customer trying to use this tool for a product and I was wondering if this issue will be fixed soon?

[rpadilla6]

@dougturnkey I wouldn't wait for this to be patched, could take a minute. If you can live with the 1.1.2 version I downgraded to that and got it running without the fs error.

[csimpi]

Nothing?

[iacapuca]

Hey, any hope?

[ilancohen]

Is there any reason we couldn't just replace the usage of geojsonhint with this:
https://www.npmjs.com/package/geojson-validation

After all, geojsonhint is used in only one place, here:

  api.add = function (geojson) {
    const errors = geojsonhint.hint(geojson, { precisionWarning: false }).filter(e => e.level !== 'message');
    if (errors.length) {
      throw new Error(errors[0].message);
    }

[ericscottmarquez]

Yeah... why would this get published with this error...

[ericscottmarquez]

I'm having the same issue with fs as OP stated

[ericscottmarquez]

In reference to #626 (comment),

Doing:

  node: {
    fs: "empty"
  }

(in the webpack config)
really shouldn't be a valid solution...