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

Moving to Openlayers 5 #123

Closed
SamuelIrungu opened this issue Jun 16, 2018 · 18 comments
Closed

Moving to Openlayers 5 #123

SamuelIrungu opened this issue Jun 16, 2018 · 18 comments

Comments

@SamuelIrungu
Copy link
Collaborator

SamuelIrungu commented Jun 16, 2018

We have some legacy openlayers animation here , also here, here

@Viglino
Copy link
Owner

Viglino commented Jun 17, 2018

Yes its old ol version (<3.20).
I think we can remove it for good.
I'll try tout push it this week.
@+

@SamuelIrungu
Copy link
Collaborator Author

SamuelIrungu commented Jun 18, 2018

Yeah, or maybe we can update them to view.animate in current openlayers, I will be working on a new branch to make ol-ext compatible with ol 5 which has breaking changes.

@Viglino
Copy link
Owner

Viglino commented Jun 18, 2018

The view.animate is already used. There is just a fallback when it doesn't exit (older version). We just have to remove the test.

I can see you're working on an ol-5 branch 👍 . Be aware we have to provided a full build (./dist) for those who use it. This means we have to

I think we can still use: import ol_control_Control from 'ol/control/Control'. The name is internal to the file and doesn't have any effect on the rest of the code, but we can easily convert it to ol.control.Control.
If you use import Control from 'ol/control/control' we can't guest it is an ol.control.Control.
According to this we just have to verify the CamelCase rule. The default exports of v4.x will still work though.

import ol_Map from 'ol/Map';

export function createMap() {
  return new ol_Map({});
}

Do you have a repo as ol-ol-ext-webpack-example with to work on it.
@+

@SamuelIrungu
Copy link
Collaborator Author

SamuelIrungu commented Jun 18, 2018

Oh, I never saw the guide, also I was wrong on using import Control from 'ol/control/control' which will make it impossible to generate the dist ol-ext version. Maybe I can lay low abit and wait for official ol-5 release. What do you think?

@Viglino
Copy link
Owner

Viglino commented Jun 18, 2018

I think we can test the pre-release v5.0.0-beta.10 to see if all is running well and what is broken.
I've test a bit and it seems that some default exports are missing. I don't know if it will be the case in the final release (but it seems to be).
For example, ol is no more exported and we need to import the inherits method:

import ol from 'ol'
...
ol.inherits(ol_control_Search, ol_control_Control);

should be:

import {inherits as ol_inherits} from 'ol'
...
ol_inherits(ol_control_Search, ol_control_Control);

I've the same issue with all ol namespaces: ol.proj, ol.coordinate, etc.

import {add as ol_coordinate_add} from 'ol/coordinate'
import {transform as ol_proj_transform} from 'ol/proj'
...
ol_coordinate_add(coord, 5);
var c = ol_proj_transform(coord, 'EPSG:4326', 'EPSG:4358');

All other classes (with CamelCase) should work properly with no modification as they are exported as default (I guess).

I don't know how it will run with jsDoc and I propose to stay with the classical namespace (ol.coordinate) still we get an other idea 😄.

@SamuelIrungu
Copy link
Collaborator Author

SamuelIrungu commented Jun 18, 2018

Yeah you are right, I think the only things to change is inherits , ol.proj and ol.coordinate . Maybe you can do a global refactoring of the imports and we can have a ol-ext-2.1.0-beta . Maybe you can do a new branch and I will try searching for errors.

@Viglino
Copy link
Owner

Viglino commented Jun 18, 2018

I've already done some test on your example repo. I can remove the error on the ol-ext lib but I've still get some warning on ol itself.
The Swipe control doesn't work properly but I don't know if it is the pre-release or not.

Viglino added a commit that referenced this issue Jun 27, 2018
Viglino added a commit that referenced this issue Jun 27, 2018
@Viglino
Copy link
Owner

Viglino commented Jun 27, 2018

Hello,
I've been working on restoring the example with the last ol version (now v5!).
The main issue was on deprecated functionalities and the removal of the optional opt_this argument on Observable:

obs.on('change', fn, this)

is now

obs.on('change', fn.bind(this));

So far all is working with v4.6.5 and v5.0.0 (with the dist) excepted the ol.control.Profil that is using an ol.Sphere that is no more available in ol5 (use ol/sphere functions instead) and all examples are running.

Now I'll been working on moving to ol5 but this will introduce breaking changes with the ol4
I've created a new branch ol-5 to handle changes (main changes concerned inherits , ol.proj, ol.coordinate and ol/sphere + CamelCase modules).

@+

@Viglino Viglino changed the title Fix to old version openlayers animation Moving to Openlayers 5 Jun 27, 2018
@SamuelIrungu
Copy link
Collaborator Author

SamuelIrungu commented Jun 28, 2018

This is great work @Viglino , I will create a test app with ol-ext@v2.0.5 and ol5 to do more tests. Will later upgrade my main projects to ol5. Thanks much.

for info ol5 is officially out

@Viglino
Copy link
Owner

Viglino commented Jun 28, 2018

Use the ol-5 branch to test.
The last commit includes a first test to have the src/control/Profile.js working (see fc702b9#diff-3cce39e052bcb5bd5955f32dbe81c470).
You can see the main import changes:

import {inherits as ol_inherits} from 'ol'
import {getDistance as ol_sphere_getDistance} from 'ol/sphere'
import {transform as ol_proj_transform} from 'ol/proj'

import ol_control_Control from 'ol/control/Control'
import ol_Feature from 'ol/Feature'

@SamuelIrungu
Copy link
Collaborator Author

@Viglino in the ol-migration, all refactoring is done and I think all migration is done except for ol_coordinate including GeomUtils. You can maybe work on that, otherwise there should be few errors and we can fix as we go

@Viglino
Copy link
Owner

Viglino commented Jul 13, 2018

I've done some tests and it seems ok to me.
I'll try to publish a new npm version (2.0.6) for use with ol<5 tomorrow.
Then we can merge the branch to master and publish a v3 to use with ol5.

@Viglino
Copy link
Owner

Viglino commented Jul 14, 2018

Hello,
I've publish a new v2.0.6 on npm.
@DarkScript you can merge your branch to the master to prepare a new version (v3) to use with ol5 😃
@+

@SamuelIrungu
Copy link
Collaborator Author

SamuelIrungu commented Jul 14, 2018

Hi,
@Viglino I pulled all updates from ol-migration to master(master up-to-date now), did some refactoring to ol_coordinate and this will probably break some of your examples, I didn't generate dist folder

@Viglino
Copy link
Owner

Viglino commented Jul 14, 2018

OK I'll have a look!

@Viglino
Copy link
Owner

Viglino commented Jul 14, 2018

I've build the dist and the examples are running (except snapguide).
I'll try to investigate later...

@Viglino
Copy link
Owner

Viglino commented Jul 14, 2018

All examples are running 😄
I'll try to test modules and I think we can publish a new npm version.

@Viglino
Copy link
Owner

Viglino commented Jul 18, 2018

New version v3.0.0 is now available on npm ol-ext@3.0.0 😄

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

No branches or pull requests

2 participants