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

Regression: 0.45.0 breaks with "." in source ID #6660

Closed
mike-marcacci opened this issue May 13, 2018 · 3 comments
Closed

Regression: 0.45.0 breaks with "." in source ID #6660

mike-marcacci opened this issue May 13, 2018 · 3 comments
Assignees
Labels

Comments

@mike-marcacci
Copy link
Contributor

mike-marcacci commented May 13, 2018

Beginning with 0.45.0, having a "." in a layer's ID crashes the web workers. I originally thought this was the same as #6648, but it isn't:

From this comment:

I'm experiencing this too. On upgrading to 0.45.0 from 0.45.2 I get several errors in the console in Chrome (I suspect one for each worker):

this.parent.getWorkerSource(...)[a[2]] is not a function
    at Ma.receive (blob:http://localhost:3000/c3408085-1173-4234-bfef-47ecdda464e3:1)

image

Mapbox is imported with import mapboxgl from "mapbox-gl"; and pulls in the minified /dist/mapbox-gl.js. I've also tried importing mapbox-gl/dist/mapbox-gl-dev.js, which has the same effect. The problem line appears to be this line.

From this comment:

Ah, ok, so the problem is the const keys = data.type.split('.'); As of 0.45.0 it has become illegal to use a period character in your source/layer names. This is definitely a bug, as this is a pattern we use pretty extensively.

Mapbox should probably escape periods (and any other special characters) before serializing, and then unescape them when deserializing.

EDIT: looking at the git blame, it appears that this has been in place for a while... perhaps something else has caused it to become problematic just now?

EDIT2: Ok, so it looks like the bug was likely introduced in 373f5c4

mapbox-gl-js version: 0.45.0

browser: Chrome 66.0.3359.170

Steps to Trigger Behavior

  1. upgrade to 0.45.0
  2. add layers with a period in the name (ex: "mygroup.mylayer")
  3. run it

Expected Behavior

Expected (v0.44.0)

Actual Behavior

Actual (v0.45.0)

Uncaught TypeError: this.parent.getWorkerSource(...)[a[2]] is not a function

EDIT: I had switched the "Actual" and "Expected" links above. 🤦🏻‍♂️

@anandthakker
Copy link
Contributor

Ah, thanks @mike-marcacci! And yes, I think you're right that 373f5c4#diff-cab2e47d8b0edbe014f919bc172194b2 introduced the problem: before that, the data.type property was comprised of source type and method name, neither of which would have included a .

@anandthakker anandthakker changed the title Regression: 0.45.0 breaks with "." in layer ID Regression: 0.45.0 breaks with "." in source ID May 14, 2018
@anandthakker
Copy link
Contributor

Note: retitled the issue to refer to source ID, rather than layer ID. They happen to be the same when source data is provided as part of the layer definition, as in the OP's examples, but the underlying issue is about the source ID.

@ChrisLoer ChrisLoer self-assigned this May 18, 2018
ChrisLoer added a commit that referenced this issue May 18, 2018
ChrisLoer added a commit that referenced this issue May 18, 2018
ChrisLoer added a commit that referenced this issue May 19, 2018
ChrisLoer added a commit that referenced this issue May 21, 2018
@ChrisLoer
Copy link
Contributor

Fixed on master with #6700.

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

No branches or pull requests

3 participants