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

Enable tree shaking #316

Closed
fergiemcdowall opened this issue Dec 5, 2023 · 14 comments · Fixed by #317
Closed

Enable tree shaking #316

fergiemcdowall opened this issue Dec 5, 2023 · 14 comments · Fixed by #317

Comments

@fergiemcdowall
Copy link
Owner

fergiemcdowall commented Dec 5, 2023

I am trying to tree shake stopword

At the moment, when using a bundler such as Webpack or Rollup stopword will always include all lists of stopwords whether they are used or not. This means that bundled applications that include stopword will often be much larger than necessary.

Example
In this gist, the compiled output of npm test includes all languages even though it should only include English stopwords.

The Fix
Updating package.json as follows ->

{
// ...
  "main": "./src/stopword.js",
  "module": "./src/stopword.js",
  "browser": "./src/stopword.js",
// ...
}

However- we have to make sure that the jsdeliver version continues to be generated

@fergiemcdowall
Copy link
Owner Author

If I understand correctly, we only have to keep the /dist folder as-is and publish to npm in order for it to appear on jsdelivr?

@fergiemcdowall
Copy link
Owner Author

On second thoughts, we only need to change one line in package.json:

  "browser": "./src/stopword.js"

That said, given that the whole module is now written in ES6, there is probably no good reason not to also set this for main and module

fergiemcdowall added a commit that referenced this issue Dec 5, 2023
@fergiemcdowall
Copy link
Owner Author

published a release candidate for testing purposes -> [email protected]

@eklem
Copy link
Collaborator

eklem commented Dec 12, 2023

I'm not sure, but it may be that part of the change made Jsdelivr use go to almost zero.

Screenshot 2023-12-12 at 07 18 30

Jsdelivr statistics was going down quite a lot, but not in that order of magnitude. Anyway, it could be coincidental. The most used file in v2.0.8 looks identical in v3.0.0-rc1.

https://cdn.jsdelivr.net/npm/[email protected]/dist/stopword.umd.min.js
https://cdn.jsdelivr.net/npm/[email protected]/dist/stopword.umd.min.js

@fergiemcdowall
Copy link
Owner Author

Hmm- strange. Publishing to npm with the tag next should have no effect on jsdelivr, but maybe I am wrong?

I can see that the value of https://cdn.jsdelivr.net/npm/[email protected] is incorrect. It should probably be the same as https://cdn.jsdelivr.net/npm/[email protected] (pointing to the umd file in /dist).

I will upload a 3.0.0-rc2 version where package.json includes the line "jsdelivr": "./dist/stopword.umd.js", as per the docs. Hopefully that will fix https://cdn.jsdelivr.net/npm/[email protected] which will then make downloads work again.

Its worth a try at least! 😅

@fergiemcdowall
Copy link
Owner Author

https://cdn.jsdelivr.net/npm/[email protected] looks better, so maybe downloads will pick up again..?😅

@eklem
Copy link
Collaborator

eklem commented Dec 12, 2023

Nice! Hopefully it will =)

@eklem
Copy link
Collaborator

eklem commented Dec 19, 2023

I'm guessing that either the change had nothing to do with the decline in use of JsDelivr as CDN, or that it was one user that has switched to another way of using the library. Or if it wasn't important for them, they have found another solution.

@fergiemcdowall
Copy link
Owner Author

Its very strange that the downloads should change so much, but yes, we may just have lost one "megauser"

@eklem
Copy link
Collaborator

eklem commented Dec 28, 2023

I think you can release it as a proper v3.0 now.

@fergiemcdowall
Copy link
Owner Author

OK 👍

@fergiemcdowall fergiemcdowall linked a pull request Jan 2, 2024 that will close this issue
@fergiemcdowall
Copy link
Owner Author

Just published on npm (will propagate out to jsdelivr)

@eklem
Copy link
Collaborator

eklem commented Jan 2, 2024

Is this issue because of the change?
#318

@fergiemcdowall
Copy link
Owner Author

Possibly- will take a look.

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 a pull request may close this issue.

2 participants