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

MapillaryJS's minified production build seems to be broken #283

Closed
goncalo-godwitlabs opened this issue Sep 4, 2018 · 8 comments
Closed
Assignees
Milestone

Comments

@goncalo-godwitlabs
Copy link

goncalo-godwitlabs commented Sep 4, 2018

Hello,

Thanks for mapillary-js and Mapillary in general, this is great stuff.

I am experiencing a small issue integrating mapillary-js in a React app and running the production build. It seems to me an issue with mapillary-js since I cannot reproduce it with the other dependencies, but I am not sure, it could be something with my setup. Also found a workaround I describe below. Follows the issue:

Basic information

MapillaryJS version: 2.12.1
System/Browser: Ubuntu linux running Firefox 61.0.1 (64-bit) and Chrome 68.0.3440.106 (Official Build) (64-bit)

Steps to Reproduce Behavior

 1. git clone git@github.com:goncalo-godwitlabs/mply.git && cd mply
 2. npm install
 3. npm run build
 4. serve -s build
 5. firefox http://localhost:5000

Expected behavior

The web app is expected to start just as if run in dev mode with npm start which runs without issues.

Actual behavior

The app raises the following exception:

Uncaught TypeError: e is not a function
    at mapillary.min.js:formatted:56028
    at mapillary.min.js:formatted:56003
    at mapillary.min.js:formatted:56004
    at Object.287../lib/Promise (mapillary.min.js:formatted:56070)
    at o (mapillary.min.js:formatted:18613)
    at mapillary.min.js:formatted:18616
    at Object.331.../../Component (mapillary.min.js:formatted:60027)
    at o (mapillary.min.js:formatted:18613)
    at mapillary.min.js:formatted:18616
    at Object.290../component/AttributionComponent (mapillary.min.js:formatted:56163)

image

Workaround

Build the app with mapillary non-minified source by changing the respective package.json entry from "main": "dist/mapillary.min" to "main": "dist/mapillary".

@oscarlorentzon
Copy link
Member

Thanks for reporting and providing the steps and example repository to reproduce. The minified distribution should be working. Good that you found a workaround for now.

@danieldunderfelt
Copy link

danieldunderfelt commented Nov 19, 2018

I used patch-package to automatically patch Mapillary-js's package.json file with this workaround solution and it works well. Be advised that you need version 6 (beta) of patch-package to get rid of the package.json default exclusion. Here's my patch file:

diff --git a/node_modules/mapillary-js/package.json b/node_modules/mapillary-js/package.json
index 1b422c8..0c05c0b 100644
--- a/node_modules/mapillary-js/package.json
+++ b/node_modules/mapillary-js/package.json
@@ -2,7 +2,7 @@
   "name": "mapillary-js",
   "version": "2.14.0",
   "description": "WebGL JavaScript library for displaying street level imagery from mapillary.com",
-  "main": "dist/mapillary.min",
+  "main": "dist/mapillary.js",
   "license": "MIT",
   "homepage": "https://github.com/mapillary/mapillary-js#readme",
   "keywords": [

Put it in patches/mapillary-js+2.14.0.patch.

@oscarlorentzon when will this be fixed? MapillaryJS is fundamentally broken in production, which is bad since its hard to notice while developing. I want to be clear that this patch is a very short-term solution and relies on the end-user having a minification step in their build process.

I can probably get the time to do a PR if the fix isn't imminent, but that will be after a week or so.

@oscarlorentzon
Copy link
Member

@danieldunderfelt Thanks for the update.

@goncalo-godwitlabs @danieldunderfelt Can you explain what is different in a react production build compared to a development build? Is there some optimization happening and in that case what does it do?

@danieldunderfelt
Copy link

@oscarlorentzon The error is in MapillaryJS's minified production build that is distributed along with the npm install, so it's not relevant whether the project uses React or not. This issue is a bit mistitled. Using the unminified version of mapillaryJS works, which is what the workaround does. The error that leads to the minified build failing needs to be addressed, it is not enough to just use the unminified script.

@oscarlorentzon
Copy link
Member

@danieldunderfelt I see. Reason for asking is that all the examples in the documentation uses the minified javascript file and there everything works as expected. Therefore I figured that the React production build does something during its its build steps with the dependencies it includes.

@danieldunderfelt Is your error message the same as the one @goncalo-godwitlabs gets?

In any case, I will investigate the issue.

@danieldunderfelt
Copy link

@oscarlorentzon Maybe the npm distribution uses an older version, and the one distributed through script tags works? Try installing mapillary-js with npm and see if it works.

The error message I got was slightly different, about t.setValue being undefined or not a function. But the workaround works in my case too.

@oscarlorentzon
Copy link
Member

@danieldunderfelt Will try, thanks for the info.

@goncalo-godwitlabs goncalo-godwitlabs changed the title On integrating mapillary-js in a React app and running a production build MapillaryJS's minified production build seems to be broken Nov 19, 2018
@goncalo-godwitlabs
Copy link
Author

goncalo-godwitlabs commented Nov 19, 2018

I have changed the issue title, I guess it makes more sense now, thanks @danieldunderfelt.

@oscarlorentzon oscarlorentzon added this to the 2.15.0 milestone Dec 6, 2018
oscarlorentzon added a commit that referenced this issue Dec 11, 2018
Fixes #283

Compressing during minification causes React and Angular
app production builds to throw error in Threejs
rendeBufferDirect method where a function is missing.
Removing compression causes about a 5 percent increase
in minified bundle size.
@oscarlorentzon oscarlorentzon modified the milestones: 2.15.0, 2.14.1 Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants