feature: Better exports#126
Conversation
This has the aim of exposing the existing API without using a single barrel file, so that e.g. the http-utils functions can be sensibly used by other packages or so that a bundler-friendly import of individual modules can be used. The existing exports as defined in the base index.js have been modularised into indidual files under api/. Everything is still re-exported in index.js ensuring the existing exported API is still available without change, but adding the new individual exports.
ejn
force-pushed
the
feature/better-exports
branch
from
9c191ef to
790f6a4
Compare
|
Sorry for the noise - now tidied up the lint problems and rebased for a clean history |
|
Thank you! I'm on mobile currently so I'll give this a more complete review later. I have some questions/remarks already:
Thank you! |
|
|
(and thanks for the quick reply - I've tried to answer quickly as I won't be able to for the next few days: but I understand that this PR is in some way a fairly big change so I was expecting some discussion!) |
jahow
left a comment
jahow
left a comment
There was a problem hiding this comment.
Ok so I think this approach is good and it'd be great to have it in the next release.
As for entrypoints I think it'd be great to just have these:
ogc-client/shared[.js](containing everything under /shared including http-utils, cache, etc. for the sake of simplicity)ogc-client/ogc-api[.js]ogc-client/wms[.js]ogc-client/wmts[.js]ogc-client/wfs[.js]ogc-client/tms[.js]- ...and
ogc-client/stac[.js]when #129 is merged
Importing things from ogc-client[/index.js] should obviously still work!
Note the ability to point to actual JS files which is important when working without a bundler.
If you don't have the capacity to update this PR I can also do it, and anyway we'll rebase this once #129 is merged so that we can add the new path accordingly.
| @@ -0,0 +1,5 @@ | |||
| export { | |||
| check, | |||
There was a problem hiding this comment.
we can probably drop this to be honest, I can't remember why it's part of the API in the first place; to be investigated more
There was a problem hiding this comment.
I guess officially that would be an API break and a new major version since this function is already exposed?
Maybe in the first instance keep the export but mark it as @deprecated?
There was a problem hiding this comment.
it would be a major version yes, but I think we're heading that way because we dropped support for older versions of node
| export { | ||
| sharedFetch, | ||
| setFetchOptions, | ||
| setQueryParams, |
There was a problem hiding this comment.
I would not put this in the API, even though it's useful it's also specific to what we do inside the library and we need toe flexibility to change its behavior without notice
There was a problem hiding this comment.
The other functions already exposed from http-utils should stay though?
Since re-using this function was my original motivation for this then it's a shame, but I understand your reasons for not wanting it to be part of the API: I guess I'll just have to copy and paste it into my local library instead.
There was a problem hiding this comment.
well we might also leave it exported but mark it as not part of the official API; then we still have the freedom of changing it in the future!
Could you describe a bit more your use case? maybe we can expose something a bit more high level in the API too.
There was a problem hiding this comment.
I had to look what I'm actually doing with it! Looks like building up custom WFS queries in a scenario where I'm not wanting to first retrieve the capabilities (capabilities are retrieved once when the layer is added and the configuration saved - when the map is re-opened then no new capabilities request is made).
I think I actually replaced a local function I had with this function since it is a nice implementation of all the special cases needed for dealing with OWS GET requests, particularly case-sensitive parameter names, and was better than what I previously had.
For such nice general-purpose functions I always find it a shame when libraries don't expose them for re-use, although I can understand the argument of wanting to keep the API surface higher-level.
I assume, regarding your comment below, now not including
Agreed.
Is this really necessary when those working without a bundler can import directly from
I'm happy to keep working on this - from your feedback we're only really talking about minor changes here. |
If the code contains imports like {
'ogc-client/': 'https://path-to-libs/ogc-client/'
}Whereas if the import is {
'ogc-client': 'https://path-to-libs/ogc-client/index.js',
'ogc-client/wfs': 'https://path-to-libs/ogc-client/wfs.js',
// etc... all other internals of ogc-client must be added here
}So I think it makes sense to allow the first kind of imports? I'm not 100% sure though. |
I've no idea really either since my use-case is always with a bundler, but I can't see any real disadvantages to having the exports all as |
2 participants
This has the aim of exposing the existing API without using a single barrel file, so that e.g. the http-utils functions can be sensibly used by other packages or so that a bundler-friendly import of individual modules can be used.
The existing exports as defined in the base index.js have been modularised into indidual files under api/. Everything is still re-exported in index.js ensuring the existing exported API is still available without change, but adding the new individual exports to be used for those who want them.
As a bonus the function
setQueryParams()fromhttp-utilsis exposed as this could be helpful for users of this library. Actually this was the reason I started on this PR, the rest was just trying to do things better :)