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

Downloads: if item is a portal item and it is multilayer, export all layers #427

Merged
merged 2 commits into from
Nov 16, 2020

Conversation

rgwozdz
Copy link
Contributor

@rgwozdz rgwozdz commented Nov 13, 2020

Download export requests for multilayer, portal items should export ALL layers. This PR ensures that occurs by remove the "layers" property from the exportItem call.

Copy link
Contributor

@r00b r00b left a comment

Choose a reason for hiding this comment

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

LGTM but someone more experienced that I should review the dep changes

Comment on lines -13 to -15
"@esri/arcgis-rest-feature-layer": "^2.14.1",
"@esri/arcgis-rest-portal": "^2.14.1",
"@esri/arcgis-rest-request": "^2.14.1",
Copy link
Member

Choose a reason for hiding this comment

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

I'm glad to see these become peerDepenencies, but Unfortunately, this will manifest as breaking change b/c people will now need to install these.

Also, I'm considering consolidating this package w/ hub-content (#306), which would solve this issue.

I think we should update the versions, but leave them as dependencies for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -42,7 +42,12 @@ export function portalRequestDatasetExport(

function composeExportParameters(params: any) {
const { datasetId, spatialRefId, where } = params;
const layerId = datasetId.split("_")[1] || 0;
const layerId = datasetId.split("_")[1];
Copy link
Member

Choose a reason for hiding this comment

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

FYI - there's a util for this:

https://esri.github.io/hub.js/api/content/parseDatasetId/

Unfortunately it's not available to this package, let's leave a TODO comment like this

Suggested change
const layerId = datasetId.split("_")[1];
// TODO: move parseDatasetId() to hub-common and use that here
const layerId = datasetId.split("_")[1];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"devDependencies": {
"@esri/arcgis-rest-auth": "^2.14.1",
"@esri/arcgis-rest-auth": "^2.21.0",
Copy link
Member

@tomwayson tomwayson Nov 16, 2020

Choose a reason for hiding this comment

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

Also, we'll need to update these devDependencies in all other packages to match these (even if they're dependencies in this package), as per https://github.com/Esri/hub.js/wiki/Dependencies#outside-the-monorepo:

Then you would bump the version of the dependency in all existing devDependencies within the monorepo to the new minor version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

…yer, export all layers

affects: @esri/hub-oauth-demo, @esri/hub-annotations, @esri/hub-common, @esri/hub-content,
@esri/hub-downloads, @esri/hub-events, @esri/hub-initiatives, @esri/hub-search, @esri/hub-sites,
@esri/hub-surveys, @esri/hub-teams
affects: @esri/hub-downloads
Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

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

thanks @rgwozdz!

@rgwozdz rgwozdz merged commit 95e4a20 into master Nov 16, 2020
@rgwozdz rgwozdz deleted the p/downloads-export branch November 16, 2020 22:00
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