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

fix(pydeck-carto): Use local isPureObject #8880

Merged
merged 2 commits into from
May 10, 2024

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented May 9, 2024

Loading the latest version of pydeck-carto in a Python notebook environment, a function imported from @loaders.gl/core is not available, and throws an error when calling requestWithParameters:

Screenshot 2024-05-09 at 3 12 29 PM

There's some nuance in how the Python packages load dependencies from CDNs, which could benefit from a closer look, but in the meantime ... the function is very short, and maybe importing it from @loaders.gl/core is unnecessary. To resolve the bug, I've copied isObject and isPureObject into the CARTO module, both are one-liners.

Publishing a patch release of @deck.gl/carto will be enough to fix the bug — pydeck-carto does not require an update.

Background

Change List

  • Copies isObject and isPureObject from @loaders.gl/core
  • Updates unit tests

@coveralls
Copy link

coveralls commented May 9, 2024

Coverage Status

coverage: 89.822% (+0.002%) from 89.82%
when pulling 4d6433a on donmccurdy/fix-pydeck-carto-isPureObject
into 950d666 on master.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

If it fixes the problem...

@donmccurdy
Copy link
Collaborator Author

Found the cause of the bug. In the pydeck environment we have only the imports coming from the jupyter-widget build, which pulls in dependencies here...

import * as loadersExports from '@deck.gl/core/scripting/loadersgl';

... and cherry-picks ...

/**
* Re-exported loaders.gl API in the pre-built bundle
* Cherry-pick loaders core exports that are relevant to deck
*/
export {registerLoaders, load, parse, fetchFile} from '@loaders.gl/core';

... and does not include isPureObject. It think it's probably more trouble than it's worth for a one-line function to pass that through, so I'd prefer to keep this PR as-is. We should be careful about adding new imports from @loaders.gl/* in pydeck-related code, unless/until the dependency loading is simplified. It's currently difficult to test local deck.gl changes in pydeck.

Likely there will be more options for dependency management if we are moving pydeck toward Anywidget, which has good support for ESM.

@donmccurdy donmccurdy force-pushed the donmccurdy/fix-pydeck-carto-isPureObject branch from 38e3536 to 4d6433a Compare May 10, 2024 15:17
@donmccurdy donmccurdy merged commit 5270e55 into master May 10, 2024
4 checks passed
@donmccurdy donmccurdy deleted the donmccurdy/fix-pydeck-carto-isPureObject branch May 10, 2024 15:26
@ibgreen
Copy link
Collaborator

ibgreen commented May 10, 2024

Such functions should probably be in @loaders.gl/loader-utils rather than @loaders.gl/core: visgl/loaders.gl#3001

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

Successfully merging this pull request may close these issues.

3 participants