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

Set externals properly for UMD builds #129

Merged
merged 3 commits into from
Feb 28, 2018
Merged

Set externals properly for UMD builds #129

merged 3 commits into from
Feb 28, 2018

Conversation

patrickarlt
Copy link
Contributor

@patrickarlt patrickarlt commented Feb 27, 2018

Close #128.

@tomwayson @jgravois I did verify that this makes bundles smaller but you should verify that the demos still work. I probably inadvertently removed this while setting up the build system.

AFFECTS PACKAGES:
@esri/arcgis-rest-auth
@esri/arcgis-rest-common-types
@esri/arcgis-rest-feature-service
@esri/arcgis-rest-geocoder
@esri/arcgis-rest-groups
@esri/arcgis-rest-items
@esri/arcgis-rest-request

ISSUES CLOSED: #128
@tomwayson tomwayson self-assigned this Feb 27, 2018
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 2f77c9f on proper-externals into 22f84e2 on master.

@tomwayson
Copy link
Member

Demos that I've tested so far:

  • gecode-browser
  • ago-node-cli

I think if we test one of the auth ones we can merge. I have to run, but will try that later.

@Esri Esri deleted a comment from coveralls Feb 27, 2018
@Esri Esri deleted a comment from coveralls Feb 27, 2018
@Esri Esri deleted a comment from coveralls Feb 27, 2018
@jgravois
Copy link
Contributor

jgravois commented Feb 28, 2018

I think if we test one of the auth ones we can merge.

our only real auth demo uses Express (and consequently doesn't load UMDs, much like ago-node-cli). that said, my own browser geocoding demo has its own OAuth implementation for batch geocoding. i confirmed locally that (a) the UMDs did indeed shrink and (b) didn't bust anything auth related.

all said, nice catch @tomwayson and truly elegant fix @patrickarlt. i was stuck on the idea that we'd need to omit the package we actually wanted to bundle from the externals: [].

thinking about it more, your one liner makes sense because internally a package wouldn't include an import that referenced its own name.

@jgravois jgravois merged commit 78512a3 into master Feb 28, 2018
@jgravois jgravois deleted the proper-externals branch February 28, 2018 00:40
@tomwayson
Copy link
Member

duh - forgot this repo did it's own CJS builds. so glad I tested ago-node-cli! thanks @jgravois

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