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

fix: set sideEffect to false in package.json #1305

Closed
wants to merge 2 commits into from

Conversation

missing1984
Copy link

Ran into treeshaking issue in Webpack and found sideEffect is not declared in package.json. https://webpack.js.org/configuration/optimization/#optimizationsideeffects

This PR is setting sideEffect: false in package.json in order to enable Treeshaking. I've go over the code and it doesn't seems we have side effects so it should be safe to set it to false.

@missing1984 missing1984 requested a review from a team as a code owner August 26, 2022 22:25
@xDivisionByZerox
Copy link
Member

I think our UniqueStore has side effects.

See line 143, we store a result in a global store (at least if we use the default store).

if (compare(store, result) === -1 && exclude.indexOf(result) === -1) {
store[result] = result;
options.currentIterations = 0;
return result;

Or am I misinterpreting?

@xDivisionByZerox xDivisionByZerox added c: chore PR that doesn't affect the runtime behavior p: 1-normal Nothing urgent labels Aug 26, 2022
@codecov
Copy link

codecov bot commented Aug 26, 2022

Codecov Report

Merging #1305 (287cae9) into main (7b18404) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1305      +/-   ##
==========================================
- Coverage   99.63%   99.62%   -0.01%     
==========================================
  Files        2160     2160              
  Lines      240614   240614              
  Branches     1017     1013       -4     
==========================================
- Hits       239730   239706      -24     
- Misses        863      887      +24     
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/internet/user-agent.ts 81.74% <0.00%> (-6.35%) ⬇️

@missing1984
Copy link
Author

I think our UniqueStore has side effects.

See line 143, we store a result in a global store (at least if we use the default store).

if (compare(store, result) === -1 && exclude.indexOf(result) === -1) {
store[result] = result;
options.currentIterations = 0;
return result;

Or am I misinterpreting?

It's fine as this store is local to the package and does not create sideEffect outside of package scope

@xDivisionByZerox
Copy link
Member

It's fine as this store is local to the package and does not create sideEffect outside of package scope

Ok, I see. The scope is per package, my bad.

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read a bit about what that is, and yeah, looks like its file for us to set that

@ST-DDT would you like to check independent from me if you are also okay with this?
That way I cannot bias you with my knowledge

@Shinigami92 Shinigami92 requested a review from ST-DDT August 29, 2022 16:06
@ST-DDT
Copy link
Member

ST-DDT commented Aug 29, 2022

I have trouble understanding, what this requires exactly.
The documentation isn't that helpful either and some SO comments actually contradict the examples.
I need some more time and probably some tests to determine, whether this is safe to apply.

@Shinigami92
Copy link
Member

Based on my comment I send here: mitodl/open-discussions#3714 (comment)
I'm not so sure anymore if this PR would help in anyway even if we would merge it...
I'm tempted to close this and forward to our v8 solution which will fix this in a much higher order

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior needs rebase There is a merge conflict p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants