Project structure and code

[jamescoq]

Discussion about project structure and code constructs

As for the project structure, I would suggest to stick to our monorepo standard. I think we have it on all our opensource projects (like https://github.com/lundegaard/validarium) as it gives you an easy way for library extension. Since we will have here main repository and also documentation app I think it already applies for monorepo usage.

Also I would start a discussion about flattening the contents of /component folder.

Way of usage of react context. I have found this way (https://kentcdodds.com/blog/how-to-use-react-context-effectively) of using of react context quite neat and transparent. Would you consider adapting the code structuring?

[MaceM8]

Hi, thanks for creating this.

Project structure
Monorepo is a great thing, but the focus of this library is to provide react map wrapper. So far there is no plan to have multiple packages or library extension. Documentation app is on PR with its package.json, that is true, but I'm not sure that it means we need monorepo with lerna or workspaces

Flattening /components folder
Flattening of components folder is needed a bit, that is for sure. Namely SearchInput, which already has its own issue - #11.
But a categorization of components based on their main focus can be useful in future versions of library. I think we can have folders like controls (enabling map interactivity - keyboard, mouse, etc..), features (adding possibilites - clusterer, signals, etc...) and possibly elements (map elements - Pointer, Route).
This way we could have clearer distinction of what these components do. But I'm open for discussion.

Context usage
Improving usage of React context is always welcome. Great example, I'd be more than happy to adapt the code. Thanks.

TypeScript?
Also I have another topic for discussion regarding code - TypeScript
By not having it typed from the beginning we are narrowing the amount of projects where it can be used out-of-the-box, which is not what we want. Using pure JS library in TS project is possible, but it needs a bit of config, which is always something that does not have to be there. What do you think?

[tommmyy]

Hi,

Monorepo

I am 100% for the monorepo setup.

@MaceM8 u speak about future features regarding the "component" folder. I say that these future features deserve its own package.
What I see there are already candidates for packages:

• core
• utils
• docs (private)

Components

• everything in "controls" folder is called "XXXControl" I don't see need for nesting it.
• @MaceM8 we should learn from lundium and use flat src folder with smart script for publishing it:
• src/ZoomControl, src/useGetMyLocation

Babel

• I am surprised that the project even compiles. in https://github.com/lundegaard/react-mapy-cz/blob/develop/.babelrc there are no babel plugins listed for ES features used inside project. It is a new babel implicit default functionality?

Codereview

I took quick look on the code it self. Nice job overall!

Misc

• I don't like the dependency of the library on the global window.SMap object. Is it only one possible solution?

https://github.com/lundegaard/react-mapy-cz/blob/develop/src/components/MapProvider.js

• Should not we use clean up function for the useEffect? to destroy already existing map?
• Can't we have only reference to current SMap? and update the properties when they change without destroying already created instance.

https://github.com/lundegaard/react-mapy-cz/blob/develop/src/components/Marker.js

• Checking for empty object is not as easy as is currently implemented, see https://github.com/lodash/lodash/blob/4.17.15/lodash.js#L11479 But it can be OK for the project. IDN

https://github.com/lundegaard/react-mapy-cz/blob/develop/src/components/MarkerLayer.js

• ❗ you need to move creation of layer into Effect it must not be created in render itself! it can cause multitude of problems in concurrent mode.
• I dont like the name "afterCreate" should be onAfterCreate... not sure why we need this at all and why the marker layer is always recreated. It can be "code smell".

https://github.com/lundegaard/react-mapy-cz/blob/develop/src/components/MultipleMarkers.js

Probably good optimisation would be check what exactly changed int markersData instead od reacreating the group everytime the REFERENCE to array change.

https://github.com/lundegaard/react-mapy-cz/blob/develop/src/components/Signals.js

• Would be much better to use useLastestRef pattern for the callbacks https://epicreact.dev/the-latest-ref-pattern-in-react/

https://github.com/lundegaard/react-mapy-cz/blob/develop/src/components/Clusterer.js

• ❗ You cant use call markerLayer.setClusterer directly in render method. Move it to useEffect at least. Not sure why this is written like that, I don't have a experience with s-mapy. Maybe like this?

const Clusterer = () => {
  const { map } = useContext(MapContext);
  const markerLayer = useContext(MarkerLayerContext);
  const clustererRef = useRef();

  useEffect(() => {
    clustererRef.current = new window.SMap.Marker.Clusterer(map);
    return () => {
      clustererRef.current = null;
    };
  }, [map]);

  useEffect(() => {
    markerLayer.setClusterer(clusterer.current);
  });

  return null;
};

https://github.com/lundegaard/react-mapy-cz/blob/develop/src/components/SearchInput/SearchInput.js

Would not be better just use renderProp pattern for rendering the input?

The review is not complete but I hope it will be useful.

[MaceM8]

Ok, I see your points.

## Monorepo

Lets agree to use warn workspaces, but about lerna I'm still not sure if we need it. Releasing will be done only with the core package.

Components

About the flat structure - in Lundium it is there mainly because of the script that makes es folders with components and simple package.json. Here we are using experimental webpack 5 module output, so I think we should't need scripts like we have in Lundium.

Babel

webpack takes babel-loader, which uses @babel/core with basics already inside. I'm not sure if it supports unreleased features, but it works and config is minimal, so its nice :) Testing the outputted bundle and using the library will take place in very near future.

Codereview

Excellent points, thank you so much for them. I have a plan to refactor and improve most of the components, since current implementation is definitely not at the level I want it to be.
All comments will be addressed, thank you for taking a time to check it :)

[tommmyy]

Monorepo

Lerna is nice for releasing process -> creates a tag, generates changelog and publishes to npm. If you have a way to do it only with yarn I am for it.

Babel

Hm, I am still not conviced though. I would rather have explicitly named plugins than using defaults. At the time you there is no information what concrete ES standard the code base use. At least this should be configured via targets https://babeljs.io/docs/en/options#no-targets

Thank you