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

update rollup etc. and install the latest (transpiled) @terraformer/arcgis #1201

Merged
merged 2 commits into from
May 19, 2020

Conversation

jgravois
Copy link
Contributor

@jgravois jgravois commented May 18, 2020

thank you @destus90 for pointing out the problem with #1194. the error they saw in IE11 would have been caught out at compilation time if we had been running a newer version of rollup to bundle the library.

this PR is an alternative to #1199 that bumps @terraformer/arcgis to a version that is already transpiled and also updates rollup and its plugins to more modern versions so we don't miss similar errors in the future.

john@zuzu esri-leaflet (master) $ npm run build

> esri-leaflet@2.4.0 prebuild /Users/john/code/ezree/esri-leaflet
> mkdirp dist


> esri-leaflet@2.4.0 build /Users/john/code/ezree/esri-leaflet
> rollup -c profiles/debug.js & rollup -c profiles/production.js


src/EsriLeaflet.js  dist/esri-leaflet-debug.js...

src/EsriLeaflet.js  dist/esri-leaflet.js...
created dist/esri-leaflet-debug.js in 908ms
  262 |    * Apache-2.0 */
  263 |
> 264 |   const edgeIntersectsEdge = (a1, a2, b1, b2) => {
      |  ^ Unexpected token: keyword «const»
  265 |     var uaT = (b2[0] - b1[0]) * (a1[1] - b1[1]) - (b2[1] - b1[1]) * (a1[0] - b1[0]);
  266 |     var ubT = (a2[0] - a1[0]) * (a1[1] - b1[1]) - (a2[1] - a1[1]) * (a1[0] - b1[0]);
  267 |     var uB = (b2[1] - b1[1]) * (a2[0] - a1[0]) - (b2[0] - b1[0]) * (a2[1] - a1[1]);
[!] (plugin uglify) Error: Unexpected token: keyword «const»

as you can see from the error, its only uglify() that was complaining about the ES2015 syntax. that's why @destus90 targeted the production build specifically.

either way, i feel bad for introducing that bug 🐛. i forgot that i'd sprinkled a little extra ES sugar when i ported arcgis-to-geojson-utils. the good thing about @destus90's fix (and this one) is that they set you up to start using some more modern JS in this library and have it transpiled down for older browsers too if you ever feel like it.

i opened up this PR to get a little bit closer to the root of the problem, but also because i thought it would uncover the error to described in #1200.

i was only successful on the first front.

@@ -26,7 +29,8 @@ export default {
name: 'L.esri',
globals: {
'leaflet': 'L',
'esri-leaflet': 'L.esri'
'esri-leaflet': 'L.esri',
'@terraformer/arcgis': 'Terraformer'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this solves an unrelated rollup nag.

Use output.globals to specify browser global variable names corresponding to external modules
/Users/john/code/ezree/esri-leaflet/node_modules/rollup/dist/rollup.js (guessing 'arcgis')

@gavinr gavinr linked an issue May 18, 2020 that may be closed by this pull request
@gavinr gavinr requested a review from patrickarlt May 18, 2020 16:17
Copy link
Contributor

@gavinr gavinr left a comment

Choose a reason for hiding this comment

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

I tested this locally, seems to work and also works in IE11.

@patrickarlt
Copy link
Contributor

Personally my preference would be to not transpile Esri Leaflet at all and resolve this in @terraformer/arcgis. @terraformer/arcgis should only ship ready-to-use already compiled code anyway so my preference would be to fix the build process there first to ensure Esri Leaflet can get dependencies it can use.

@jgravois
Copy link
Contributor Author

jgravois commented May 18, 2020

@terraformer/arcgis should only ship ready-to-use already compiled code anyway

i've been considering it a feature that @terraformer/arcgis ships both a UMD and a flat ESM module that can be tree shaken by consumers like Esri Leaflet.

i've never considered shipping a pre-transpiled ESM before, but i will make it so.

@jgravois
Copy link
Contributor Author

jgravois commented May 18, 2020

alright, i just pushed another commit to revert the new babel 🤸 step here.

what's left

  • bump @terraformer/arcgis to the latest (complete with internal transpilation)
  • bump rollup and friends to latest
  • fix rollup nag

sorry this turned into a dogfooding session, but i appreciate you helping me improve terraformer@latest!

i don't have access to a box to test IE11 anymore, so help on that front would be appreciated.

@jgravois jgravois closed this May 18, 2020
@jgravois jgravois reopened this May 18, 2020
@jgravois jgravois changed the title update rollup and friends and use babel to transpile update rollup and friends and install the latest (transpiled) version of @terraformer/arcgis May 18, 2020
@jgravois jgravois changed the title update rollup and friends and install the latest (transpiled) version of @terraformer/arcgis update rollup etc. and install the latest (transpiled) @terraformer/arcgis May 19, 2020
@gavinr gavinr self-requested a review May 19, 2020 14:58
Copy link
Contributor

@gavinr gavinr left a comment

Choose a reason for hiding this comment

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

Looks good. I tested on IE11, works fine.

@gavinr gavinr merged commit 2dc6134 into Esri:master May 19, 2020
@jgravois jgravois deleted the patch-1198-alt branch May 19, 2020 22:31
gavinr pushed a commit to gavinr/esri-leaflet-renderers that referenced this pull request Jun 5, 2020
jgravois pushed a commit to jgravois/esri-leaflet that referenced this pull request Apr 23, 2022
update rollup etc. and install the latest (transpiled) @terraformer/arcgis
jgravois pushed a commit to jgravois/esri-leaflet that referenced this pull request Apr 23, 2022
update rollup etc. and install the latest (transpiled) @terraformer/arcgis
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.

CDN version is broken in IE11
3 participants