mapbox / mapbox-gl-draw

Draw tools for mapbox-gl-js
https://www.mapbox.com/mapbox-gl-js/example/mapbox-gl-draw/
ISC License
952 stars 593 forks source link

Request: please provide ES5 compiled code #725

Closed mgvarley closed 4 years ago

mgvarley commented 6 years ago

mapbox-gl-js version: 0.42.2 mapbox-gl-draw version: 1.0.4

Steps to Trigger Behavior

  1. Create a new react application using CreateReactApp https://github.com/facebookincubator/create-react-app
  2. npm install mapbox-gl and @mapbox/mapbox-gl-draw
  3. Create bare bones map using sample code for mapbox-gl-draw
  4. Attempt to build. mapbox-gl only works fine and compiles, add mapbox-gl-draw to the mix and it breaks the build:
Creating an optimized production build...
Failed to compile.

Failed to minify the code from this file:

        ./node_modules/@mapbox/mapbox-gl-draw/src/modes/index.js:10

Read more here: http://bit.ly/2tRViJ9

Following the link suggests the issue is due to code not published as ES5 on npm, presumably mapbox-gl code is published in this way, would be great if mapbox-gl-draw would follow the same approach:

npm run build fails to minify

Some third-party packages don't compile their code to ES5 before publishing to npm. This often causes problems in the ecosystem because neither browsers (except for most modern versions) nor some tools currently support all ES6 features. We recommend to publish code on npm as ES5 at least for a few more years.

To resolve this: Open an issue on the dependency's issue tracker and ask that the package be published pre-compiled.

Expected Behavior

A clean build when including @mapbox/mapbox-gl-draw in a project that uses create-react-app and/or UglifyJS

Actual Behavior

Including @mapbox/mapbox-gl-draw breaks the build. Just including mapbox-gl works fine.

jingsam commented 6 years ago

try import @mapbox/mapbox-gl-draw/dist/mapbox-gl-draw

mgvarley commented 6 years ago

@ jingsam that worked perfectly, thank you! Happy to submit a PR to add this into the README as a workaround but would be great if this worked as expected with import @mapbox/mapbox-gl-draw

rorysedgwick commented 6 years ago

This is also causing a problem for me which is less simple to resolve, as we are using react-mapbox-gl-draw, which is requiring @mapbox/mapbox-gl-draw itself which we can't easily overwrite.

I realise this may be an issue to filed opened against the react-mapbox-gl-draw repo but do you have any suggestions for workarounds in the meantime?

mcwhittemore commented 6 years ago

Using the complied code via import MapboxDraw from '@mapbox/mapbox-gl-draw/dist/mapbox-gl-draw'; is the currently proposed way to do this.

I'm all ears for better solutions.

timofei-iatsenko commented 6 years ago

Better solution is add to package.json different fields for different kinds of source and consumers:

 "es2015": "./esm2015/common.js",
 "main": "./bundles/common.umd.js",
 "module": "./esm5/common.js",

This should work at least for Webpack and based on it tools (AngularCLI, React Create App and etc)

mcwhittemore commented 6 years ago

@thekip could you run this out in a PR?

mcwhittemore commented 6 years ago

It might be worth looking into rollup for this kind of support.

https://rollupjs.org/guide/en

thiagoxvo commented 6 years ago

I am having a related but different problem. I am creating a new mode, but at the same time I want to reuse @mapbox/mapbox-gl-draw/src/lib/common_selectors for example, and I am not able to import it because of the same minification error during my build.

Do you have a suggestion how can I make it work in my case?

mcwhittemore commented 6 years ago

@thiagoxvo - In the spirit of keeping this ticket about shipping Draw in different flavors of JS, can you please open a different issue suggesting making common_selectors a public API. I suspect the real solution is for us to extend what default functions are on the mode_interface rather than having you require that file.

@mgvarley or @thekip (really anyone) do you have sometime to make a PR for the solution @thekip outlined?

thiagoxvo commented 6 years ago

Sure @mcwhittemore I will open another ticket.

amagee commented 6 years ago

FWIW I have been using the following script:

#!/bin/bash

# Create ES5 versions of the Mapbox Draw source files so `react-scripts build`
# will work:
# https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/template/README.md#npm-run-build-fails-to-minify
# We do for every source file in Mapbax Draw because some of our code uses
# Mapbox Draw internals that are not exposed in the main dist file.
# The `npx babel ./src -d ./dist/` sets up `./dist` as a recursive copy of
# `./src` where each file has been transpiled to ES5.

cd node_modules/@mapbox/mapbox-gl-draw
npx babel ./src -d ./dist/

# Also create an ES5 version of the index file. It has a bunch of `require`
# statements that look like `require('./src/stuff')`; we rewrite them to
# `require('./stuff')` to fit into our new structure.
sed 's/\.\/src\//\.\//' < index.js > dist/index.js
cd -

which generates ES5 versions of each of the source files. This solves @thiagoxvo 's issue:

import common_selectors from '@mapbox/mapbox-gl-draw/dist/lib/common_selectors';

works without requiring any modification to Mapbox Draw's API.

kkaefer commented 4 years ago

Fixed.

trygveaa commented 4 years ago

This isn't fixed as far as I can see. You still have to import @mapbox/mapbox-gl-draw/dist/mapbox-gl-draw. I submitted #976 as a fix for this.