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

Stop maintaining fork of ol3-google-maps #99

Open
mstenta opened this issue Feb 9, 2021 · 5 comments
Open

Stop maintaining fork of ol3-google-maps #99

mstenta opened this issue Feb 9, 2021 · 5 comments
Assignees
Labels
bug Something isn't working postponed

Comments

@mstenta
Copy link
Member

mstenta commented Feb 9, 2021

This is a bit of a thorny issue... opening this to document it for future reference... I may end up working around it in the short-term, at least until a proper upstream fix is available.

Back story:

farmOS-map includes the ol3-google-maps library for Google Map layer support (added via #8). However, when Google Layers are put in a layer group, they do not work. This is documented in this upstream issue: mapgears/ol3-google-maps#285

I dug into it and figured out the cause, described here: mapgears/ol3-google-maps#285 (comment)

I managed to fix the issue for farmOS's immediate needs, but it is not a "complete" solution, so it was not ready for a pull request to be opened and merged upstream. I didn't had time to work on it further, so I simply created a fork with a farmOS-map branch containing the fixes we needed and updated farmOS-map's package.json to point to that fork+branch.

See this commit:9cb8311

And this comment: #8 (comment)

Fast forward to today:

Now, we are trying to pull farmOS-map into farmOS 2.x via Composer (https://www.drupal.org/project/farm/issues/3186641). We decided that we need to host farmOS-map on NPM and pull it in via asset-packagist.org (the reasoning for this is described in that issue).

See related issue: #64

However, after publishing v1.3.0 to NPM, attempting to pull it in via Composer results in the following error:

$ composer require npm-asset/farmos.org--farmos-map:^1.3
./composer.json has been updated
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - npm-asset/farmos.org--farmos-map 1.x-dev requires npm-asset/github:mstenta/ol3-google-maps dev-farmos-map -> no matching package found.
    - npm-asset/farmos.org--farmos-map 1.3.0 requires npm-asset/github:mstenta/ol3-google-maps dev-farmos-map -> no matching package found.
    - Installation request for npm-asset/farmos.org--farmos-map ^1.3 -> satisfiable by npm-asset/farmos.org--farmos-map[1.3.0, 1.x-dev].

Potential causes:
 - A typo in the package name
 - The package is not available in a stable-enough version according to your minimum-stability setting
   see <https://getcomposer.org/doc/04-schema.md#minimum-stability> for more details.
 - It's a private package and you forgot to add a custom repository to find it

Read <https://getcomposer.org/doc/articles/troubleshooting.md> for further common problems.

Installation failed, reverting ./composer.json to its original content.

It seems that asset packagist does not support github fork+branch dependencies.

I found this issue, which sounds like it is the same, but no solutions: hiqdev/asset-packagist#133

Proper long-term solution

The correct solution to this is to fix mapgears/ol3-google-maps#285 upstream, so that we can simply pull in olgm via NPM normally.

Potential short-term solution

A potential short-term solution would be to publish my fork+branch on NPM so that we can reference it directly. I hate maintaining a fork like that, but we don't have the resources to dig into the proper solution right now. Ideally, we can just delete the NPM package in the future when mapgears/ol3-google-maps#285 is fixed, and collapse/simplify this whole thing (and close this issue). But for now, I think that's the approach I'm going to take...

@mstenta mstenta added the bug Something isn't working label Feb 9, 2021
@mstenta
Copy link
Member Author

mstenta commented Feb 9, 2021

publish my fork+branch on NPM so that we can reference it directly

https://www.npmjs.com/package/@farmos.org/farmos-olgm

mstenta added a commit to mstenta/farmOS-map that referenced this issue Feb 9, 2021
@mstenta mstenta mentioned this issue Feb 9, 2021
@mstenta
Copy link
Member Author

mstenta commented Feb 9, 2021

I implemented the described workaround. Renaming this issue to "Stop maintaining fork of ol3-google-maps"... to be continued...

@mstenta mstenta changed the title Fork+branch of ol3-google-maps not compatible with asset packagist Stop maintaining fork of ol3-google-maps Feb 9, 2021
symbioquine added a commit to symbioquine/farmOS-map that referenced this issue Jun 1, 2021
**Why?** The strategy of loading the Google maps API via a
script tag prior to initializing farmOS-map was one of the
primary drivers of slow loading - even when Google maps
wasn't used. See performance testing at farmOS#112

Although the strategy of splitting each behavior into its own
chunk mostly mitigates any cost of including Google maps support
in farmOS-map, it will be better for the maintainability of farmOS-map
not to directly include Google maps support (RE:
farmOS#99) and a contrib module
for farmOS is a cleaner place for the integration to live anyway since
such a contrib module could be fully optional and could include
optimizations about how/when to load the Google maps API JS.
symbioquine added a commit to symbioquine/farmOS-map that referenced this issue Jun 3, 2021
**Why?** The strategy of loading the Google maps API via a
script tag prior to initializing farmOS-map was one of the
primary drivers of slow loading - even when Google maps
wasn't used. See performance testing at farmOS#112

Although the strategy of splitting each behavior into its own
chunk mostly mitigates any cost of including Google maps support
in farmOS-map, it will be better for the maintainability of farmOS-map
not to directly include Google maps support (RE:
farmOS#99) and a contrib module
for farmOS is a cleaner place for the integration to live anyway since
such a contrib module could be fully optional and could include
optimizations about how/when to load the Google maps API JS.
symbioquine added a commit to symbioquine/farmOS-map that referenced this issue Jun 3, 2021
**Why?** The strategy of loading the Google maps API via a
script tag prior to initializing farmOS-map was one of the
primary drivers of slow loading - even when Google maps
wasn't used. See performance testing at farmOS#112

Although the strategy of splitting each behavior into its own
chunk mostly mitigates any cost of including Google maps support
in farmOS-map, it will be better for the maintainability of farmOS-map
not to directly include Google maps support (RE:
farmOS#99) and a contrib module
for farmOS is a cleaner place for the integration to live anyway since
such a contrib module could be fully optional and could include
optimizations about how/when to load the Google maps API JS.
@mstenta
Copy link
Member Author

mstenta commented Sep 6, 2021

@symbioquine: Curious how we should proceed with this issue now that the dependency on @farmos.org/farmos-olgm has been moved to https://github.com/symbioquine/farm_map_google

farmOS-map is no longer depending on it, but technically the @farmos.org organization is still "maintaining" the fork.

@symbioquine
Copy link
Collaborator

I think it's still mainly a matter of getting your issue mapgears/ol3-google-maps#285 resolved. I can try following up on that. Maybe I can find a strategy that they'll accept a PR for...

@symbioquine symbioquine self-assigned this Sep 19, 2021
@symbioquine
Copy link
Collaborator

Looks like we'll be able to stop using/supporting the farm_map_google module once we update to OpenLayers 9 - and maybe add some core behavior to support it...

https://openlayers.org/en/latest/examples/google.html
openlayers/openlayers@7245d80
openlayers/openlayers@5755b13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working postponed
Development

No branches or pull requests

2 participants