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

chore: remove src from published package #7417

Closed
wants to merge 1 commit into from

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Dec 27, 2023

This removes src/ from the published npm package.

Does anyone have any good reason we can't do this? seems it'd save us a fair amount of package size.

extracted from the published @next package, src seems to be 1.2M locally 🤔

bundlephobia doesn't show it since it isn't an export afaik

This removes `src/` from the published npm package.

Should save a fair few bytes.
@kwonoj
Copy link
Member

kwonoj commented Dec 27, 2023

We include it for the declaration map for typescript which provides better navigation to the src when develop. As said it is not included in the actual application bundle.

@kwonoj kwonoj closed this Dec 27, 2023
@43081j
Copy link
Contributor Author

43081j commented Dec 27, 2023

It is included in the published npm package.

https://unpkg.com/browse/rxjs@7.8.1/src/

When you say it isn't included in the application bundle, you mean you're relying on consumers tree shaking this stuff? They still have to unnecessarily download it in the first place

You also produce DTS files in a separate directory. So does this only get published to let you have source maps or something?

@kwonoj
Copy link
Member

kwonoj commented Dec 27, 2023

It is included in the published npm package.

Yes, that is the purpose. Check https://www.typescriptlang.org/tsconfig#declarationMap for the details.

@43081j
Copy link
Contributor Author

43081j commented Dec 27, 2023

Interesting, do you have an example of when a consumer would need this?

My understanding is these will basically allow us to see the source rather than the type definitions. Which would make sense for someone debugging rxjs itself, but not your own code afaict.

You could cut a huge amount of the package size down by dropping this. Which it seems is useful to only a small amount of people, probably contributors here.

@kwonoj
Copy link
Member

kwonoj commented Dec 27, 2023

Which it seems is useful to only a small amount of people, probably contributors here.

No, contributors won't need since contributor works on the source. Any dev uses editor, and uses typescript's lsp to navigate rxjs will see the actual source instead of single, multi line type definition only. In case of rxjs it is quite useful since lot of operators are relying on numbers of overloading signatures and just reading it won't give enough details time to time how internal actually works.

I don't deny it's an addition to the download bytesize, however these are not included in the actual application runtime and we think it gives enough benefit to the developers given past experiences from reported issues who needed to understand rxjs's behavior but not directly having source navigation in their application code.

@43081j
Copy link
Contributor Author

43081j commented Dec 27, 2023

I would question how many consumers actually read the implementation. This also suggests either the jsdoc or the signature isn't clear enough.

You may be supporting an extreme edge case here, at the cost of all consumers having to download 3x (esm, cjs, source) the package size as opposed to 2x (esm, cjs).

I think it would be worth revisiting.

Considering the 20+ million downloads a week, 1/3 of that data is probably only used by tens of people, if that.

It'd be an easier decision to make with some metrics etc but it sounds like this is more of a result of occasional issues finding it difficult to reproduce something. So I'm not sure how we can measure it properly, but I think it is worth discussion

Do you want me to make an issue instead?

@kwonoj
Copy link
Member

kwonoj commented Dec 27, 2023

We are always open to improve things and this issue was actually discussed before couple of times, and so far our conclusion stays same. Given we're trying to reorganize our code with modular, there's a chance to reduce size of src itslef which will help this naturally as well.

I can understand reason where you come from, but also as a long time maintainers we have lot of history / context to land certain conclusion to keep things in certain way. We did evaluate the point you suggested in the meantime as well. Github search is awful, but you may able to search some history in the issues as well. The couple of things we considered are things like 1. npm serves archived tarball, so the actual download size is not excessivly increased 1:1 to text size. 2. lot of pkging caching on the ci helps to avoid redownloading.

TL:DR, we'll keep things as is for now, and revisit while / after we do reorganazing codes and think about some other aspect. I do not believe making an issue is required.

@43081j
Copy link
Contributor Author

43081j commented Dec 27, 2023

You're right, compression will help so it won't actually be exactly 3x. It will still be a considerable amount of extra data though and we shouldn't justify that with the fact tools can cache npm downloads.

I don't doubt you've each put thought into this before, plenty of it. Would be good to read those discussions if they're in issues here.

Happy to leave this for now but I do doubt enough people need to read the typescript source to justify this. I'm sure we all agree the majority of consumers won't read such sources (since the majority won't be debugging this stuff). Those who do need this can still read the JS source (what I do when I debug any other package).

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.

2 participants