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

Ensure loseContext exists before calling it #4245

Merged
merged 4 commits into from
Jun 7, 2024

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jun 7, 2024

@birkskyum Similar fix was necessary to get plotly.js working with virtual-webgl (v1).
See plotly/mapbox-gl-js@71e2cfc

This is for your brilliant pull request plotly/plotly.js#7015
if you could possibly test a build with make-baselines-virtual-webgl to see if it passes on CircleCI
or alternatively add the following script to the top of plotly.js devtools/test_dashboard/index.js and then npm start to test interactively.

<script src="https://unpkg.com/browse/virtual-webgl@1.0.6/src/virtual-webgl.js"></script>

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.37%. Comparing base (d919ba3) to head (7cc8f68).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4245      +/-   ##
==========================================
- Coverage   87.73%   87.37%   -0.36%     
==========================================
  Files         242      242              
  Lines       33081    33081              
  Branches     2160     2173      +13     
==========================================
- Hits        29023    28906     -117     
- Misses       3088     3182      +94     
- Partials      970      993      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@birkskyum
Copy link
Member

birkskyum commented Jun 7, 2024

Thanks for upstreaming it!

When i run npm run start and go to gl2d heatmap, it appear to work both w/o the script tag - what should I look for interactively? Seems like it's also getting blocked on localhost.
Screenshot 2024-06-07 at 04 46 22

Either way, I think this change makes sense as it's more accurately guarding the function call.

@birkskyum birkskyum requested a review from HarelM June 7, 2024 02:49
@birkskyum
Copy link
Member

birkskyum commented Jun 7, 2024

Can you add a changelog entry like this:
- ⚠️ Ensure loseContext exists before calling it ([#4245](https://github.com/maplibre/maplibre-gl-js/pull/4245))

in the bugfix section here:

- _...Add new stuff here..._

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Harel M <harel.mazor@gmail.com>
@HarelM HarelM merged commit b9bc927 into maplibre:main Jun 7, 2024
15 checks passed
@archmoj archmoj deleted the fix-virtual-webgl branch June 10, 2024 16:06
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