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

Add OS Map Layer #758

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"static-security-scan": "npm audit --json > npm-audit-report.json",
"scan:zap": "npm run docker-security-scan && npm run static-security-scan",
"mock:api": "node ./test/mock-api/index.js",
"build": "npm run scss && cp -R src/assets/downloadable/. public/downloadable && npm run build-webpack",
"build": "npm run scss && cp -R src/assets/downloadable/. public/downloadable && cp -R src/assets/static/. public/static && npm run build-webpack",
"build-webpack": "webpack",
"serve-webpack": "webpack serve",
"scss": "sass --quiet-deps --load-path=./ src/assets/scss:public/stylesheets",
Expand Down
42 changes: 34 additions & 8 deletions src/assets/js/map.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import parse from 'wellknown'
import maplibregl from 'maplibre-gl'
import { capitalize, startCase } from 'lodash'
import { getApiToken } from './os-api-token.js'

const lineColor = '#000000'
const fillColor = '#008'
Expand Down Expand Up @@ -29,28 +30,45 @@ const popupMaxListLength = 10
*/
export class Map {
constructor (opts) {
this.bbox = opts.boundingBox ?? null
this.opts = opts
this.bbox = this.opts.boundingBox ?? null
this.map = new maplibregl.Map({
container: opts.containerId,
style: 'https://api.maptiler.com/maps/basic-v2/style.json?key=ncAXR9XEn7JgHBLguAUw',
container: this.opts.containerId,
style: this.opts.style ?? '/public/static/OS_VTS_3857_3D.json',
zoom: 11,
center: [-0.1298779, 51.4959698],
interactive: opts.interactive ?? true
interactive: this.opts.interactive ?? true,
transformRequest: (url, resourceType) => {
if (url.indexOf('api.os.uk') > -1) {
if (!/[?&]key=/.test(url)) url += '?key=null'

const requestToMake = {
url: url + '&srs=3857'
}

const token = getApiToken()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please ensure the token is fetched by awaiting getFreshApiToken() somewhere in createMapFromServerContext() (or before it's called).

It sucks that the request transform function can't be async, so no straightforward way to refresh it here 😔

Copy link
Contributor Author

@DilwoarH DilwoarH Jan 14, 2025

Choose a reason for hiding this comment

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

We call await getApiToken() in the initialisation script - with calls both

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know, unfortunately there's a race condition. getApiToken() is a synchronous function (which the initialisation code awaits; it should't). That means it returns a value immediately, possibly before the promise of getFreshApiToken() completes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see the problem - good catch, will update

requestToMake.headers = {
Authorization: 'Bearer ' + token
}

return requestToMake
}
}
})

// Add map controls
this.addControls(opts.interactive)
this.addControls(this.opts.interactive)

this.map.on('load', async () => {
// Store the first symbol layer id
this.setFirstMapLayerId()

// Add the boundary GeoJSON to the map
if (opts.boundaryGeoJsonUrl) this.addBoundaryGeoJsonToMap(opts.boundaryGeoJsonUrl)
if (this.opts.boundaryGeoJsonUrl) this.addBoundaryGeoJsonToMap(this.opts.boundaryGeoJsonUrl)

// Add layer data to map
if (opts.wktFormat) this.addWktDataToMap(opts.data)
else await this.addGeoJsonUrlsToMap(opts.data)
if (opts.wktFormat) this.addWktDataToMap(this.opts.data)
else await this.addGeoJsonUrlsToMap(this.opts.data)

// Move the map to the bounding box
if (this.bbox && this.bbox.length) this.setMapViewToBoundingBox()
Expand Down Expand Up @@ -388,6 +406,14 @@ export const createMapFromServerContext = async () => {
wktFormat: geoJsonUrl === undefined
}

// fetch initial token
try {
await getApiToken()
} catch (error) {
console.error('Error fetching OS Map API token', error)
options.style = 'https://api.maptiler.com/maps/basic-v2/style.json?key=ncAXR9XEn7JgHBLguAUw'
}
DilwoarH marked this conversation as resolved.
Show resolved Hide resolved

// if the geoJsonUrl is provided, generate the paginated GeoJSON links
if (geoJsonUrl) {
options.data = await generatePaginatedGeoJsonLinks(geoJsonUrl)
Expand Down
36 changes: 36 additions & 0 deletions src/assets/js/os-api-token.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// copied from https://github.com/digital-land/digital-land.info/blob/main/assets/javascripts/osApiToken.js

const API_ACCESS_TOKEN_URL = '/api/os/get-access-token'
let apiToken = {
access_token: '',
expires_in: 0,
issued_at: 0
}

let makingRequest = false

export const getApiToken = () => {
const tokenCheckBuffer = 30 * 1000
const tokenExpires = parseInt(apiToken.expires_in) * 1000 + parseInt(apiToken.issued_at)
if (Date.now() > tokenExpires - tokenCheckBuffer && !makingRequest) {
getFreshApiToken()
}
return apiToken.access_token
}
Comment on lines +3 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Synchronous vs. asynchronous token refresh
getApiToken triggers a background refresh but returns the current token even if it is about to expire. Consider waiting for getFreshApiToken() if the expiry window is very short, to reduce the chance of using an expired token.


export const getFreshApiToken = () => {
return new Promise((resolve, reject) => {
makingRequest = true
fetch(API_ACCESS_TOKEN_URL)
.then(res => res.json())
.then(res => {
apiToken = res
makingRequest = false
resolve(apiToken.access_token)
})
.catch(err => {
makingRequest = false
reject(err)
})
})
}
Loading
Loading