Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request: please provide ES5 compiled code #725

Closed
mgvarley opened this issue Dec 13, 2017 · 13 comments
Closed

Request: please provide ES5 compiled code #725

mgvarley opened this issue Dec 13, 2017 · 13 comments

Comments

@mgvarley
Copy link

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
Copy link

jingsam commented Dec 14, 2017

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

@mgvarley
Copy link
Author

@ 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
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

@thekip could you run this out in a PR?

@mcwhittemore
Copy link
Contributor

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

https://rollupjs.org/guide/en

@thiagoxvo
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

Sure @mcwhittemore I will open another ticket.

@amagee
Copy link

amagee commented Aug 7, 2018

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
Copy link
Contributor

kkaefer commented Jan 7, 2020

Fixed.

@kkaefer kkaefer closed this as completed Jan 7, 2020
@trygveaa
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants