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

Tidy up the package.json #35

Merged
merged 3 commits into from
Jun 9, 2022
Merged

Conversation

rowanwins
Copy link
Contributor

This PR fixes up some of the pointers in the package.json.
Currently the stac-layer.min.js is 1.3MB. I'm pretty sure it's bundling all the dependencies such as leaflet.js etc in a way that means they might not being tree-shaken.
This PR should mostly resolve the issues in this repo, but I'm going to open some similar PR's upstream in georaster packages to try and fix some similar issues (eg GeoTIFF/georaster#65).

@DanielJDufour
Copy link
Member

I can't remember why main was originally set to "dist/stac-layer.min.js". I just know I've similarly set main to a build when there was some issue with a dependency using a more recent JS syntax like ??= or something like that. Sometimes implementers of stac-layer might be using something like create-react-app that doesn't support all the most recent cool JS syntax. However, I think Google Chrome now supports ??=, so this probably isn't going to be a common problem. I think you can keep "main": "src/index.js", in the PR, but just wanted to share with you the original reasoning in case this comes up in the future. (And also because I'm likely to forget my original reasoning unless I write it down!)

Could you also set jsdelivr to "dist/stac-layer.min.js" (which you may already know is what ObservableHQ uses for their default AMD loading)? This isn't required for merging because it wasn't there originally, but it would be a nice to have.

I think you mentioned this in another thread, but the bulk of the build is probably coming from https://www.npmjs.com/package/proj4js-definitions. Just wanted to mention this here in case anyone else is interested and reading this.

I intentionally specified all the files in "files: [...]" instead of using files: "["src", "dist"]" for a couple reasons: I use pkg-ok in my publishing workflow and I know it works with files. (I haven't tried with directories, so it's "probably" not a problem for this reason). The most important reason, is that I want to be intentional about which files are included and make sure I don't include any other files in the build. I usually have a lot of development files where I'm trying out new things or want to save some code to reference later. I save them in .bak files so they are ignored by git: https://github.com/stac-utils/stac-layer/blob/main/.gitignore#L107
Screen Shot 2022-04-23 at 9 45 42 PM
I also generally directly specify the files, because I don't want to accidentally include any personal information/files in a npm package. If we don't specify the files, I'll probably have to add a manual step of running npm pack and manually inspecting the list of which files are going to be published. Long story short, if it's not too much an inconvenience, I'd prefer keeping files the way it is. I only have a medium-level of preference knowing it's mostly about my own weird setup and personal preference, so if you have a strong opinion on it, I'm happy to accommodate and accept the change :-)

@m-mohr
Copy link
Collaborator

m-mohr commented Apr 24, 2022

Yeah, I think we put the .min file in there as we had issues with vue-cli/webpack bundling the new syntax of the Nullish coalescing operator etc. I'm not sure whether this has been solved yet. We should check this so that we can ensure nothing breaks, because gaining the mentioned benefits would be great.

@m-mohr
Copy link
Collaborator

m-mohr commented Jun 5, 2022

Actually, I think we should not care whether downstream there's an issue. If there's one, we need to fix it downstream. It's really a bad idea to fix something here that in reality should be fixed somewhere else. Let's merge this and then we'll figure things out in STAC Browser once released. This should be released as 0.11.0 and not as 0.10.2 ideally.

Edit: Did some tests and this seems to work after upgrading some dependencies in STAC Browser.

@m-mohr m-mohr requested a review from DanielJDufour June 5, 2022 16:49
@m-mohr m-mohr mentioned this pull request Jun 5, 2022
@m-mohr m-mohr added this to the 0.11.0 milestone Jun 5, 2022
@DanielJDufour DanielJDufour merged commit 4900d11 into stac-utils:main Jun 9, 2022
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

Successfully merging this pull request may close these issues.

3 participants