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

Fix webpack support #2857

Closed
wants to merge 2 commits into from
Closed

Fix webpack support #2857

wants to merge 2 commits into from

Conversation

michaelhue
Copy link

@michaelhue michaelhue commented Jul 12, 2016

Fixes a known issue with anonymous functions and workerify-webpack in webpack-based build environments. See original issue.

Actual error message:

blob:http://localhost:3000/e44fa451-b7fd-4eb3-871c-e5f0a502f7dd:12
Uncaught TypeError: Cannot read property 'call' of undefined

There is a known caveat with workerify-webpack which can result in
errors in some webpack environments; the worker function passed to
workerify should not be anonymous.

This one-line change names the function 'MapboxGlWorker' and resolves
the issue.
Adds a note about avoiding 'eval'-related devtool settings in webpack
configurations, which can result in errors with workerify-webpack.
@@ -14,7 +14,7 @@ var rewind = require('geojson-rewind');
var GeoJSONWrapper = require('./geojson_wrapper');
var vtpbf = require('vt-pbf');

module.exports = function(self) {
module.exports = function MapboxGlWorker(self) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand borisirota/webworkify-webpack#9 correctly, this shouldn't be necessary with webworkify-webpack 1.1.1, correct?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fixed, correct. However, I'm on webworkify-webpack@1.1.1 and mapbox-gl@0.20.1 using the recommended webpack.config.js setup. This produced the error mentioned in the pull request. Adding a name to the function fixed the issue for me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this line ☝️

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the time being, webworkify-webpack needs a named function to operate correctly.

@lucaswoj
Copy link
Contributor

If naming the function is the only way to fix webpack then I think we should do it. There are no substantial cons.

  • add a source-level comment explaining why this function must be named
  • file an upstream bug against workerify-webpack
  • create an automated test of building GL JS with webpack
  • merge this PR

@austinhyde
Copy link

I submitted borisirota/webworkify-webpack#13 yesterday, and v1.1.3 appears to fix the js/source/worker export, at least for me. Can anyone else confirm this?

@bensleveritt
Copy link

bensleveritt commented Jul 14, 2016

Dang, you beat me @austinhyde.

In addition to that I had to remove eval from devtool in my webpack config, as per webworkify-webpack's caveats.

But yes, I can confirm this shouldn't be an issue with 1.1.3+.

@lucaswoj
Copy link
Contributor

Great! Can we close this issue now @austinhyde @bensleveritt @michaelhue?

@austinhyde
Copy link

I think that if @michaelhue can confirm webworkify-webpack v1.1.3 fixes his issue, then yes. Otherwise, this is a good stopgap with no drawbacks, until it can be addressed in webworkify-webpack.

@austinhyde
Copy link

Hmm... just ran into this issue: borisirota/webworkify-webpack#13 (comment)

I'd hold off on closing/merging this for now.

@michaelhue
Copy link
Author

@austinhyde 1.1.3 works for me now, even with minified builds. Thank you! And thank you @lucaswoj for considering the issue. I think the added note about devtool compatibility in webpack.config.example.js could still be useful though.

@lucaswoj
Copy link
Contributor

Sounds like the upstream fix is underway. I prefer to wait for that!

@lucaswoj
Copy link
Contributor

I think the added note about devtool compatibility in webpack.config.example.js could still be useful though.

Just re-read this. I agree.

@lucaswoj lucaswoj reopened this Jul 14, 2016
@@ -59,5 +60,6 @@ module.exports = {
loader: 'transform',
query: 'brfs'
}]
}
},
devtool: 'cheap-source-map' // don't use eval-related options
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why inline-source-map over source-map orinline-source-map?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found cheap-source-map to be a good tradeoff but of course any other non-eval setting will do.

@austinhyde
Copy link

I'm actually going to recommend merging this. There's some bigger issues going on in webworkify-webpack that I'm trying to work through, and this is a harmless fix in the meantime.

@lucaswoj lucaswoj mentioned this pull request Jul 18, 2016
@lucaswoj
Copy link
Contributor

Replaced by #2887 (bringing the PR within the Mapbox organization to make final edits easier)

@lucaswoj lucaswoj closed this Jul 18, 2016
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

Successfully merging this pull request may close these issues.

5 participants