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

Have uglifyjs spit out a sourcemap for leaflet.js too. #5351

Merged
merged 3 commits into from
Feb 21, 2017

Conversation

danzel
Copy link
Member

@danzel danzel commented Feb 21, 2017

Fixes #5341
The resulting source map points back to the original files too 👍

@danzel
Copy link
Member Author

danzel commented Feb 21, 2017

re my second commit.
I noticed that the resulting map file for -src is called leaflet-src.js.map, but the publish.sh file was adding leaflet-src.map (the 1.0.3 tagged build has -src.map as the file name).
Not sure if that was broken or what was going on, looks like it is a recent change.

Copy link
Member

@perliedman perliedman left a comment

Choose a reason for hiding this comment

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

LGTM! @IvanSanchez do you have any comments on the extensions of the source maps before merging?

@IvanSanchez
Copy link
Member

@perliedman As I said in #5341 I don't really see the usefulness, but if you and @danzel agree, then it's OK.

@perliedman
Copy link
Member

perliedman commented Feb 21, 2017

@IvanSanchez ok, right. But the question was more about @danzel's last commit, and the actual file extension of the source map. It appears to have been just .map, compared to .js.map, and I'm unsure if this is important to anyone.

@IvanSanchez
Copy link
Member

IvanSanchez commented Feb 21, 2017

Ah, oh, ok 😄

According to the sourcemap spec:

Optionally, a source map will have the same name as the generated file but with a “.map” extension. For example, for “page.js” a source map named “page.js.map” would be generated.

So ideally I'd go for leaflet-src.js.map and leaflet.js.map.

Let me double-check @danzel 's changes to the build scripts, I'm not sure this is what we have now.

@IvanSanchez
Copy link
Member

ivan@~/devel/Leaflet\ [pr/5351]$ ls -l dist                                                                                                              
total 1836                                                                                                                                                              
drwxr-xr-x 1 ivan ivan    146 des.  23 12:44 images
-rw-r--r-- 1 ivan ivan  13888 feb.   8 16:08 leaflet.css
-rw-r--r-- 1 ivan ivan 134439 feb.  21 13:41 leaflet.js
-rw-r--r-- 1 ivan ivan 617195 feb.  21 13:41 leaflet.js.map
-rw-r--r-- 1 ivan ivan 380727 feb.  21 13:41 leaflet-src.js
-rw-r--r-- 1 ivan ivan 725203 feb.  21 13:41 leaflet-src.js.map

LGTM.

@IvanSanchez IvanSanchez merged commit bd957ad into Leaflet:master Feb 21, 2017
ghybs added a commit to ghybs/Leaflet.FeatureGroup.SubGroup that referenced this pull request Mar 26, 2017
in order to comply with source map spec
https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit#heading=h.9ppdoan5f016
> Optionally, a source map will have the same name as the generated file but with a “.map” extension.  For example, for “page.js” a source map named “page.js.map” would be generated.
See also: Leaflet/Leaflet#5351
ghybs added a commit to ghybs/Leaflet.MarkerCluster.LayerSupport that referenced this pull request May 13, 2017
in order to comply with source map spec
https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit#heading=h.9ppdoan5f016
> Optionally, a source map will have the same name as the generated file but with a “.map” extension.  For example, for “page.js” a source map named “page.js.map” would be generated.
See also: Leaflet/Leaflet#5351
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.

4 participants