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

Enable running Loam from a CDN like unpkg #76

Merged
merged 3 commits into from
Sep 14, 2021
Merged

Conversation

ddohler
Copy link
Collaborator

@ddohler ddohler commented Jul 29, 2021

Overview

This turned into a bit of a grab-bag, but it does two key things to improve developer experience:

  • Enables running Loam from a CDN (e.g. unpkg). This closes Allow passing exact file path to loam.initialize() #58 and will make it easier to run Loam from CodePen and similar.
  • Creates a demo page that can be run by executing yarn demo. This provides a complete example of how to utilize loam in a simple webapp, and can be used as a test bed for features (in fact, it uses unpkg to load GDAL) or as a playground.

Notes

  • The way that this achieves the CDN loading is not ideal, for the reasons described in the comment in the code. I think I've done enough research to convince myself that this is the best way forward, but I would be very happy to have a discussion about the approach to double-check.

Testing Instructions

  • Run yarn install to pick up dependency upgrades.
  • Running tests should all pass (CI is handling this, but you can follow the directions in the README to replicate)
  • Running yarn demo should result in a demo page being available on localhost:8080 -- you should be able to edit Loam source or the page source and change the behavior of the page.

Checklist

  • Add entry to CHANGELOG.md
  • Update the README with any function signature changes

Resolves #58

ddohler added 2 commits July 29, 2021 14:44
Loam.initialize() now accepts an absolute path as a prefix, and accepts
an optional, second prefix for GDAL assets, instead of using the same
prefix for both Loam and GDAL assets.
@ddohler ddohler requested a review from alkamin July 29, 2021 18:47
@ddohler ddohler marked this pull request as draft July 29, 2021 18:47
@ddohler ddohler marked this pull request as ready for review August 10, 2021 19:51
@@ -14,7 +14,7 @@ jobs:

strategy:
matrix:
node-version: [10.x, 12.x]
node-version: [12.x, 14.x]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is fallout from upgrading webpack, which was required to resolve a bug and add the demo page.

@@ -50,136 +50,124 @@ self.Module = {
// Optimized builds contain a .js.mem file which is loaded asynchronously;
// this waits until that has finished before performing further setup.
onRuntimeInitialized: function () {
try {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The change here is removing this try block and delegating error handling to the native onerror handler.

},
};

// Load gdal.js. This will populate the Module object, and then call
// Module.onRuntimeInitialized() when it is ready for user code to interact with it.
importScripts('gdal.js');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This now happens within the blobified webworker initialization code.

@@ -14,33 +59,75 @@ function getPathPrefix() {
return THIS_SCRIPT.src.substring(0, THIS_SCRIPT.src.lastIndexOf('/')) + '/';
}

// Destroy the worker and clear the promise so that calling initWorker will make a new one.
function clearWorker() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was necessary because the statefulness of the worker made testing initialization processes impossible without a way to reset the worker state.

throw new Error('render() promise should have been rejected but got ' +
result + ' instead.'
throw new Error(
'render() promise should have been rejected but got ' +
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added format-on-save to my editor since the last time touching this file and there was some minor fallout.

@ddohler
Copy link
Collaborator Author

ddohler commented Aug 11, 2021

This is ready for review (and was yesterday, but I was rushing to get it done before I had to sign off and forgot to specify)!

Copy link

@alkamin alkamin left a comment

Choose a reason for hiding this comment

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

I couldn't formulate a better approach to this that also avoids the multi-stage initialization sort of approach than what you're doing here. Works nicely.

### `loam.reset()`
Tear down Loam's internal Web Worker. This will cause initialize() to create a new Web Worker the next time it is called.

**Note**: This exists primarily to enable certain types of unit testing. It should not be necessary to call this function during normal usage of Loam. If you find that you are encountering a problem that loam.reset() solves, please [open an issue](https://github.com/azavea/loam/issues)
Copy link

Choose a reason for hiding this comment

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

Appreciate the warning here.

This was much trickier than anticipated due to
webpack/webpack-dev-server#2484
which necessitated upgrading to webpack-dev-server 4.0.0-rc.0
@ddohler ddohler force-pushed the feature/dpd/run-from-cdn branch from 1e564fd to 60c5a29 Compare September 14, 2021 19:38
@ddohler ddohler merged commit 05a0af0 into develop Sep 14, 2021
@ddohler ddohler deleted the feature/dpd/run-from-cdn branch September 14, 2021 19:39
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.

2 participants