Scope Improves an existing behavior Compatibili

<a class="user-mention notranslate" data-hovercard-type="user" data-hover

Given the following code: <div class="highlight highlight-text-html-basic notransl

Thanks for proposing this, <a class="user-mention notranslate" data-hovercard-type="us

Avoid barrel file exports,about mswjs/msw

Comments (44)

thepassle commented on June 11, 2024 9

Here's the PR that removes imports to a barrel file by an internally used module #1987

Leads to a pretty nice change already:
Before:

After:

from msw.

thepassle commented on June 11, 2024 3

@thepassle what tool did you use to visualize the imports ?

Madge

from msw.

mattcosta7 commented on June 11, 2024 3

Conceptually, I like the idea of removing barrels, and very much agree we shouldn't import through them in the library itself. But it's hard to have a conversation in the abstract, when it's not clear what level of size could be avoided if we did remove them

Tools like material-ui and lodash tend to provide dual import apis, with both subpath exports and a barrel file. Something like that might make sense here, but the effort/value tradeoff seems high for now? I think spliting out the handlers like http and graphql might be a small place where that could make sense, and then having a msw/core that has the rest of the core imports, might provide some value? I'm not sure how much, though, could actually be removed in those cases? Do we know what level of impact would be possible here? That might help to weight the cost/benefit a bit to see whether it's worth taking on?

from msw.

thepassle commented on June 11, 2024 3

Given the following code:

<html>
  <body>
    <script type="module">
      import { http } from 'msw';
      import { setupWorker } from 'msw/browser';

      const worker = setupWorker();
      await worker
        .start({
          serviceWorker: {
            url: './mockServiceWorker.js',
          },
        });
      worker.use(http.get('/api/foo', () => Response.json({ foo: 'foo' })));

      const response = await fetch('/api/foo').then(r => r.json());
    </script>
  </body>
</html>

Leads to the following:

185 of those requests are caused by the barrel file. 1.3mb goes unused. This is on my fast m1 macbook as well, this will be a lot slower in CI. This also only considers MSW exclusively.

We have about 20-40 things exported from the root of msw (the core module). Does it mean we will be introducing 20-40 individual export paths for all those things?

No, I don't think thats necessary, but we can make a more granular split of entrypoints.

Like you say, test runners don't usually bundle the code, so you will still end up importing everything from that package, which was precisely my point.

Right, I see where you're going with this now :) We in fact avoid barrel files in general (not only for dev-only libraries, but also production libraries), because we use a microfrontend architecture and load features/libraries from a CDN (similar to module federation, but not quite the same), and barrel files don't perform well when loaded from a CDN.

We are at the stage of discussion and search for a progressive solution to this. Swapping the root-level exports is not a progressive solution.

I disagree! Msw actually has a great opportunity to take a leading position in the ecosystem and setting a great example for other libraries to follow :)

That's a huge breaking change that may take years to properly roll out.

This is really not the case, as I mentioned imports can very easily be codemodded, and we can add more granular entrypoints, while keeping the main barrel file in this major version in place. I did mention this above, as well.

I've not seen a single package do that in all the time I'm doing open source. Does anybody have an example of a package solving this problem that way, perhaps?

Not open source, unfortunately, but a good example of this is a project (a library that's only used for local development and unit tests) we have at work; a few years ago I noticed my unit tests were for some reason loading all of the Storybook UI code, going entirely unused. The reason for this was barrel files. We had a storybookFixture function, that renders a Storybook story, so we can run unit tests against this, and because of a barrel file, ended up loading all of storybooks UI code.

Another example is a library we have that is used in production, (this is kind of a large library, also consumed from the CDN) so we made a sensible grouping of things that go together:

from msw.

thepassle commented on June 11, 2024 1

The latter; the way MSW exposes exports to users. E.g.: currently if you import http from "msw", you import everything in MSW. I recommend giving this a good read for the reasoning why: https://marvinh.dev/blog/speeding-up-javascript-ecosystem-part-7/

Removing the barrel file entirely would be a breaking change, which I can understand is undesirable, especially having just done a big breaking change (although changes to imports often can easily be codemodded for users). However, what we can do is already add separate/grouped/more granular entrypoints so people can import only what they use and need, and keep the barrel file in place for compatibility and avoiding a breaking change. And then on a next major version we could decide if we want to drop the barrel file entirely (which arguably, I think would be better for everyone because it would improve perf all across the board, not just in the browser), but thats something i'll leave up to you :)

from msw.

kettanaito commented on June 11, 2024 1

I guess we see things differently then, for me anything that slows local development down and can be avoided, should be avoided, especially if they are quick-wins.

I thought you were proposing the rework of the public API—the way people import http and friends. That's not a quick win though. That's a huge breaking change that may take years to properly roll out.

As far as I'm aware react-query is a library meant to be used in production code; I think the assumption to use a bundler there is more reasonable.

I fail to see the point here. You can have an integration test of a code that relies on react-query. Like you say, test runners don't usually bundle the code, so you will still end up importing everything from that package, which was precisely my point.

Additionally, "other projects have barrel files too" is not really a good argument

I don't mean to use other projects' behavior as an excuse to do or not to do something. I was simply bringing it up to illustrate that we should see and learn how others handle things. Some libraries may be times larger than MSW (pre-build) and if there are no issues reported about them slowing things down, then perhaps we are talking about a premature optimization here.

Thats fair enough, but I hope you can reconsider

We aren't at the stage of saying yes or no. We are at the stage of discussion and search for a progressive solution to this. Swapping the root-level exports is not a progressive solution. Neither is having both approaches allowed, which will only confuse the developer and their TypeScript:

import { http } from 'msw'
import { http } from 'msw/http'

You have to also consider how MSW is built to allow proper type references across different entrypoints. It's achieved by those entrypoints referencing the core build. I have a concern that with the msw/http suggestion, its types (and more) will be duplicated in that export (the whole point of not loading everything from a barrel file), which would make things incompatible during type-checking and runtime as well (think of instanceof situations).

This is not an easy problem to solve. Perhaps that's partially why the ecosystem suffers from it so greatly. While addressing this on the package level is likely the "perfect" solution, I encourage to put our effort into exploring the "progressive" solution, which we can adopt since the next release and already bring value.

I guess we see things differently then, for me anything that slows local development down and can be avoided, should be avoided, especially if they are quick-wins.

Circling back to this, do you have any metrics that proves MSW slows you down by loading a bunch of imports from a barrel file? I'd much like to see numbers, and load time numbers, not MBs downloaded (I mentioned this before, downloading 1MB on your local machine may be extremely fast). Without concrete proof of performance degradation, any conversation about performance improvements becomes a premature conversation.

Edit: nuance is sometimes hard to express in text, but just want to clarify that this discussion is all in good faith :)

I concur, respect, and appreciate that! The same from my side.

from msw.

jacobrask commented on June 11, 2024 1

Updating imports is something that a codemod should be able to do very reliably fwiw.

You have to consider the implications of this change that live outside of your code base too. Educational materials, articles, examples, docs. It's a gigantic change. It has to have gigantic benefits. As of now, I'm yet to see evidence that would suggest that does.

Yes I respect that, just my 2 cents for a personal preference of not using barrel files in general, while recognizing that the trade offs in this case involve way more data points.

from msw.

43081j commented on June 11, 2024 1

it would be a fairly large breaking change to remove the index file as a whole, so i think it makes sense to keep that around for sake of not having to update all the docs, references, etc. consumers who bundle or are entirely server-side can continue using the "pretty" import too (the bare specifier).

rxjs and many others provide groupings of exports in individual entrypoints. it may be worth doing something similar here if the current index affects unbundled consumers too much (though not a small job since discussion needs to happen to decide what those groupings are)

alternatively, you could probably just update the export map to export all the individual files so consumers have the option to directly import individual modules if they really want to.

either way, seems no loss to existing consumers who don't care about this stuff (perf, size, primary entrypoints, etc will all be the same).

from msw.

thepassle commented on June 11, 2024 1

so you can do this

You can't do this, because of the package exports

from msw.

kettanaito commented on June 11, 2024

Thanks for proposing this, @thepassle! Totally support this, let's improve the way we import (and export, if talking about our ecosystem packages) things.

from msw.

kettanaito commented on June 11, 2024

Fixed by #1987. 👏

from msw.

thepassle commented on June 11, 2024

Sorry, that PR just addresses one of the problems with barrel files; internal modules importing from a barrel file.

The issue itself still exists: the barrel file. Importing http from "msw" leads you to importing everything in MSW, most of which largely goes unused, so we should still consider splitting up imports into separate(/grouped) entrypoints

from msw.

kettanaito commented on June 11, 2024

@thepassle, do you have the internal build entrypoints in mind (i.e. tsup config) or actually changing the way MSW exports things? Like msw/http? I have some concerns against the latter.

from msw.

scr2em commented on June 11, 2024

@thepassle what tool did you use to visualize the imports ?

from msw.

kettanaito commented on June 11, 2024

@thepassle, I will give that article a proper read. For now, sharing my initial thoughts.

While imports like msw/http and msw/graphql solve this problem by providing more targeted entrpoints, this isn't a nice developer experience.

import { http } from 'msw/http'
import { graphql } from 'msw/graphql'
import { setupWorker } from 'msw/worker'

This is a lot of typing to achieve a simple thing. I was quite against msw/browser and msw/node as well but the benefit they provide runtime- and type-wise persuaded me to adopt that approach.

The benefit of msw/http is very slim, imo. I expect your compiler to tree-shake everything that ends up imported from msw that you don't use. I know you are working in a compiler-free environment so perhaps that's the reason why that's not happening (not trying to suggest a compiler to you, just asking)? As much as I respect that choice, compilers have become the norm and they are here to stay. All the frameworks I know rely on a compiler (Astro, Remix, Vue, Angular, Next). People use MSW in those frameworks so it's reasonable to expect the compiler to help with the dead code.

My second point is that MSW is a devtool. While nobody wants 1MB of JavaScript being pulled by a random third-party tool, you have to consider the implications of it. 1MB on your local machine that gets fetched almost instantly and 1MB in production are two quite different things. I see little risk of large file sizes for developer tools. This is one thing we can openly benefit from, unlike third parties that also ship to production and have to constantly chase smaller build footprints. I most certainly wish to burden the end consumer as little as possible but if it's at a cost of developer experience, I'd say it's not worth it.

from msw.

kettanaito commented on June 11, 2024

Fundamentally, this has the same problem as literally any other tooling:

import { a } from 'react-query'

Most packages are structured as a barrel file. So I pull everything now from react-query. Happy to be proven wrong here.

from msw.

mattcosta7 commented on June 11, 2024

Depending on build tool/environment, export {} from might all get lumped together and not fully shaken, especially if it's possible there are sideffects. I believe any cjs build will do this because cjs can have dynamic getters which require static other code.

Browser ESM is file-by-file imports of every file imported. They can't parse something they haven't imported, so treeshaking can't work in a browser environment (like a compiler-less solution)

Generally these can add up, but we can probably get most value by limiting barrel files.

Internally, only keep 1, exports.ts that exports a public api, and otherwise avoid using them internally (also never allow internal imports from that/those file(s)), so the number of hops is minimal? that's probably a good 80/20 solution for now

from msw.

mattcosta7 commented on June 11, 2024

Fundamentally, this has the same problem as literally any other tooling:

import { a } from 'react-query'
Most packages are structured as a barrel file. So I pull everything now from react-query. Happy to be proven wrong here.

While this is current state of the world, package exports are quite young, and I would hope more packages move away from them over time (although this is probably an idealistic assumption).

from msw.

thepassle commented on June 11, 2024

this isn't a nice developer experience.

Hm, I feel this is subjective. I personally wouldn't mind these imports, especially if it means it means my development/ci will be faster.

I expect your compiler to tree-shake everything that ends up imported from msw that you don't use. I know you are working in a compiler-free environment so perhaps that's the reason why that's not happening (not trying to suggest a compiler to you, just asking)?

I don't use a compiler, I develop in the browser

As much as I respect that choice, compilers have become the norm and they are here to stay. All the frameworks I know rely on a compiler (Astro, Remix, Vue, Angular, Next). People use MSW in those frameworks so it's reasonable to expect the compiler to help with the dead code.

People can use compilers/transpilers to transpile their source code, but do they do this during local development and in unit tests? And do they also compile their unit test code itself, where they import/setup MSW? Because if not, they're loading a lot of unused, un-treeshaken modules. I know many test runners (one example of where MSW is typically used), like jest, karma, WTR, and more, where test code is not bundled, and not treeshaken.

compilers have become the norm and they are here to stay

I'm not arguing this for production code, but they're much less common place in development tooling, like test runners.

I see little risk of large file sizes for developer tools

I guess we see things differently then, for me anything that slows local development down and can be avoided, should be avoided, especially if they are quick-wins.

Most packages are structured as a barrel file. So I pull everything now from react-query. Happy to be proven wrong here.

As far as I'm aware react-query is a library meant to be used in production code; I think the assumption to use a bundler there is more reasonable. For local development, there are many cases where code doesn't get bundled.

Additionally, "other projects have barrel files too" is not really a good argument; it just shows that barrel files are a larger problem in the ecosystem.

I'd say it's not worth it.

Thats fair enough, but I hope you can reconsider :) It'd be unfortunate to have to fork MSW again, I'd much more prefer to use the main package and contribute to improve things here, but the amount of modules we're currently loading in the browser is way too much, and it could be a lot less.

Edit: nuance is sometimes hard to express in text, but just want to clarify that this discussion is all in good faith :)

from msw.

jacobrask commented on June 11, 2024

Updating imports is something that a codemod should be able to do very reliably fwiw.

I think there will be people who dislike this and that there’s a risk for a backlash, but I’d personally agree with this change.

I fold all imports by default and use my editor/TypeScript's auto import feature.
Splitting imports on multiple lines make git diffs easier to manage.
Barrel files are bad for bundler performance as well.

from msw.

kettanaito commented on June 11, 2024

I wonder where does this change end though? We have about 20-40 things exported from the root of msw (the core module). Does it mean we will be introducing 20-40 individual export paths for all those things?

I've not seen a single package do that in all the time I'm doing open source. Does anybody have an example of a package solving this problem that way, perhaps?

from msw.

kettanaito commented on June 11, 2024

Updating imports is something that a codemod should be able to do very reliably fwiw.

You have to consider the implications of this change that live outside of your code base too. Educational materials, articles, examples, docs. It's a gigantic change. It has to have gigantic benefits. As of now, I'm yet to see evidence that would suggest that does.

Splitting imports on multiple lines make git diffs easier to manage.

This is quite subjective and rather insignificant gain from the library's perspective.

Barrel files are bad for bundler performance as well.

They are. But MSW isn't really meant to be bundled as you don't include it in your production build. I may presume we are talking about bundling the app for testing, and in that case I still need to see any tangible performance metrics that would suggest MSW causes any test run time degradation.

from msw.

derekr commented on June 11, 2024

This is a super naive setup for testing, was just curious since I've been playing with node's native test runner and have experienced issues with barrel style imports adding a non-trivial amount of overhead to each test and overall test run. Death by 1000 cuts. In this case it might be negligible, but not representative of real world test setups where maybe there is more of a performance hit. Ran on a ~2 year old apple m1 max MacBook pro.

https://github.com/derekr/mswtest

node --test src/*.test.js
mocks/handlers.js imports: 0.03ms
mocks/server.js imports: 0.002ms
▶ msw server
  ✔ should return a 200 status code (5.94925ms)
▶ msw server (6.555791ms)

mocks/handlers.js imports: 0.03ms
mocks/server.js imports: 0.002ms
▶ msw server
  ✔ should return a 200 status code (5.535209ms)
▶ msw server (6.163708ms)

ℹ tests 2
ℹ suites 2
ℹ pass 2
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 121.173958

from msw.

thepassle commented on June 11, 2024

alternatively, you could probably just update the export map to export all the individual files so consumers have the option to directly import individual modules if they really want to.

To clarify, you don't have to manually add entrypoints for every single file, you can just use a wildcard, here's how I do this in app-tools:

from msw.

kettanaito commented on June 11, 2024

@thepassle, thank you for providing some concrete metrics.

185 of those requests are caused by the barrel file. 1.3mb goes unused. This is on my fast m1 macbook as well, this will be a lot slower in CI.

Based on the screenshot, the page still loaded under 400ms. I'd consider this fine in terms of developer experience. I also wonder what the browser should do with third-party modules it fetches over HTTP. I'd expect it to cache them for they are never meant to change (you don't own them so for you they are de-facto static assets that can be cached by package version). With caching, this problem really happens just once per dependency update. But I suspect the whole direction is so new there's no actual standardization as to how the module fetching should behave (happy to be proven wrong, please post links).

No, I don't think thats necessary, but we can make a more granular split of entrypoints.

What will be the deciding factor for those entrpoints? Do we base those primarily on the amount of code each entrypoint introduces? Then I wonder how you intent to keep that amount of code in check so a new feature would not blow up a particular entrypoint.

Here we enter an even more uncharted land. Changes like this cannot really happen in a sustainable fashion unless there's tooling around them. Tooling to automat, detect, report enrtypoint size changes, tooling to work with the code in the context of where that code ends up, etc. We haven't even touched on this in the discussion but the maintainer's cost for this change is stupendous.

This brings back memories to proper package bundling and how I have to design a bunch of scripts just to make sure my package isn't broken when published and provides the right exports. I expect the tooling is about at the same state when it comes to managing module fetching and everything related to that.

I disagree! Msw actually has a great opportunity to take a leading position in the ecosystem and setting a great example for other libraries to follow :)

Even the introduction of the Fetch API has been met with a palpable struggle from the users. And I consider that a mild API change, to be frank. If the choice we make will negatively impact the end developer, then it's a choice that has to be considered with utmost thought.

I hear codemods being mentioned and I want to clarify on them. Codemods are great. But you need to know how to use codemods. Then, codemods can produce broken code. Now, you need to be aware how it's broken and have the capacity to fix it by yourself. This discussion has no doubt gathered a lot of engineers who tackle quite complex tasks. But you have to acknowledge that not every engineer is used to this level of complexity. For many, even a trivial change can be a significant time investment, and so it has to have solid justification.

Proposal

So, there are a couple of major factors that are present in the "problem":

Fetching a lot of modules even if you need only some of them;
Consequentially, ending up with MBs of unused JavaScript that hurt runtime performance.

May I propose an alternative approach here? It won't solve the problem but it will mitigate the symptoms and bring immediate value without being a breaking change.

Why don't we minify the browser bundle? We still ship the source code and sourcemaps so debugging won't suffer. With minification, we can get a significant reduction of the download JavaScript in terms of file size.

For this to work properly, we'd likely have to also bundle core and this means a completely separate build output just for the browser. Depending on how detached this gets setup-wise, this may still be worth it.

from msw.

thepassle commented on June 11, 2024

Then, codemods can produce broken code

This is extremely unlikely if the only thing that changes are imports. Also, this would only even be relevant if we would do a breaking change, which is not what we're suggesting; we're suggesting adding additional entrypoints. We can keep the current barrel file in place, for the time being.

Even the introduction of the Fetch API has been met with a palpable struggle from the users.

How is adding additional, optional entrypoints that would lead to better performance a struggle for users?

What will be the deciding factor for those entrpoints? Do we base those primarily on the amount of code each entrypoint introduces?

There are several ways we can go about this (which we can all discuss in detail, and some options have already been mentioned in this issue), that, frankly, I really don't feel put a stupendous maintenance burden on the project, I'm really not sure where you're getting this from.

Why don't we minify the browser bundle?

Because the browser bundle is not the problem, I feel I've mentioned this several times. The msw/browser entrypoint is fine as is right now (~~only 1 of those 185 requests is the browser bundle~~, EDIT: this was incorrect, it's about 8, which is acceptable). The problem is that you also need the http handler, which isn't exported from anywhere but the barrel file (the "msw" import), which leads to lots of unused imports/JS being loaded.

All in all, I really fail to understand the reluctance to this request, and I haven't really heard any solid arguments to not consider adding some additional, optional entrypoints for people who prefer to have performant local development/unit tests. I'd really love to understand better why you're so hesitant about this.

from msw.

43081j commented on June 11, 2024

i feel like this is too easily overcomplicated.

there seem to be two options:

introduce extra entrypoints (whether they are to group common exports, or a more granular level - whatever)
update the export map to export existing modules via a wildcard export (e.g. ./* or wherever they exist right now)

the former takes effort to spec it out, decide what the correct entrypoints are, etc. it also then takes extra effort to maintain and keep that in sync. it is what libraries like rxjs do and works well, but is understandable if the maintainers here don't want to take that on.

the latter is a one-time piece of work which introduces no extra burden, no breaking changes, no rfc-level discussions needed. though it does require more work from the consumer to import the exact paths of the modules they want (rather than it coming from a "nice" grouped entrypoint).

if there's this much concern about the former, i'd just update the export map and note it down in the docs somewhere.

i could be missing something, of course. let me know if i am or if i misunderstood

from msw.

mattcosta7 commented on June 11, 2024

update the export map to export existing modules via a wildcard export (e.g. ./* or wherever they exist right now)
the latter is a one-time piece of work which introduces no extra burden, no breaking changes, no rfc-level discussions needed. though it does require more work from the consumer to import the exact paths of the modules they want (rather than it coming from a "nice" grouped entrypoint).

if there's this much concern about the former, i'd just update the export map and note it down in the docs somewhere.

i could be missing something, of course. let me know if i am or if i misunderstood

This would make any change to internal folder/file structure becomes a breaking change to the public api, so I don't think that's actually as simple as you suggest

from msw.

43081j commented on June 11, 2024

This would make any change to internal folder/file structure becomes a breaking change to the public api, so I don't think that's actually as simple as you suggest

is that not just a matter of definition? you can define that the individual modules may change any time, so are not part of the "public API" (i.e. the exported namespace). consumers can choose to depend on them but at their own risk.

i've seen a few packages do this before (and, in fact, pre-export-maps most packages changed files often without a breaking change)

from msw.

kettanaito commented on June 11, 2024

How is adding additional, optional entrypoints that would lead to better performance a struggle for users?

I might have expressed my point weakly because this isn't an appropriate solution:

import { http } from 'msw'
import { http } from 'msw/http'

This contributes to a terrible experience for everyone. This confuses TypeScript and your IDE, and I can hardly imagine a proper way to document it not to cause further confusion. This is the direction I'm encouraging us to avoid, not adopt.

I really don't feel put a stupendous maintenance burden on the project, I'm really not sure where you're getting this from.

I will give you an example. Someone adds a feature. The code for that feature adds a whole new block of modules to the library. The person forgets to care about imports, and suddenly, introduces a barrel file. The person reviewing the change misses this also. Suddenly, we are back at square one. It doesn't matter if it's a top-level barrel file or a barrel file nested 50 modules deep. This is why I stress that without a proper tooling to automate this, this will always be a burden on maintainers.

You look at this as a quick solution to a problem. I look at this as a solution I would have to maintain for years. The difference in commitment level is what spawns this discussion.

We are discussing a complex problem of the entire ecosystem that nobody has quite solved yet. You must forgive any reluctancy on my part.

The problem is that you also need the http handler, which isn't exported from anywhere but the barrel file

With that suggestion, I have proposed to bundle everything for the browser. No more browser/core separate files. One browser.js that has everything minified. This can reduce the number of network requests and reduce the MBs to download. What do you think about this approach?

All in all, I really fail to understand the reluctance to this request

Not all apparent solutions are the right solutions. I believe you see a problem and see a solution for that problem. I also see numerous implications to that solution, experience-wise, runtime-wise, and documentation-wise. I have employed my expertise on this kind of thing for years, and I'm not going to sway now. I'm a strong opponent of workaround-driven development. The msw/http proposal is a workaround. Because if it's not, you are suggesting that at some point MSW should only ship exports like this, which I don't find a good experience, bundle size or not. I'd much rather see exports represent logical intention than an internal concern of how many modules they will pull into the import graph.

To be absolutely frank, I'm also a strong proponent of "measure before you cut". I want to hear more people having this issue. Because if things have been working fine for them for 5 years, there is no issue in practice.

from msw.

kettanaito commented on June 11, 2024

Has anyone considered importing things from msw/lib directly? As @thepassle has pointed out, the core part isn't bundled, so you can do this:

import { http } from 'msw/lib/core/http'
import { HttpResponse } from 'msw/lib/core/HttpResponse'

You can follow this pattern for any public export we have. This looks and works quite similar to what is being proposed with msw/http. Does this solve the issue?

I agree with @mattcosta7 that this way the emitted folder structure becomes a part of the public API. However, I'm not going to recommend these imports to anyone by default. In fact, I'd treat relying on them the same as relying on internals. We cannot guarantee their structure so they may break at any point.

I understand this isn't nice. But if we are looking at a small percentage of issue occurrence (always happy to be proven wrong, share your use cases!), then the price justifies the gain.

from msw.

mattcosta7 commented on June 11, 2024

so you can do this

You can't do this, because of the package exports

Yeah, we would need to add subpath exports like "./lib/": "./lib/..."

from msw.

kettanaito commented on June 11, 2024

I have a feeling this is slowly becoming unproductive. I suggest we approach the issue objectively, then define all possible solutions there are, weight their pros and cons, and, eventually, arrive at the right one.

I will try my best to do just that.

The problem

Barrel files pull in a bunch of unused code.
~~The problem is relevant only in a compiler-less setup~~. (a good point from @thepassle that it's not specific to the compiler setup)

Proposed solutions

Add additional entrypoints

import { http } from 'msw/http'
import { HttpResponse } from 'msw/HttpResponse'

Pros:

Import only what you need.
Consequentially, improved performance.

Cons:

Confuses the IDE since now two things are available at two different places.
Even the minimal usage of the library becomes verbose.
Puts mental overhead "what should I import from where" and "what's the difference between two same things imported from different exports" (familiarity issue).

Compromise:

We can implement this on the export conditions level only. This way the source code remains in tact but provides you pointers to standalone modules.

{
  "exports": {
    "http": "./lib/core/http.mjs"
  }
}

If this was the proposal all along, then I have missed the point entirely. I've not read "export condition" once in this discussion.

This also has its disadvantages. Not all tooling understands exports conditions. But since we are jumping ahead into the bright future of JavaScript, perhaps we can disregard this disadvantage.

Another disadvantage is the mental overhead of deciding and maintaining those root-level exports. Should all root-level APIs from msw be represented in standalone exports? Does it truly solve the problem if http imports from a barrel file 20 imports-deep? If not, then how are we defining the area of the problem and how do we intend to keep it at bay? (this is what I struggle to understand the most).

from msw.

kettanaito commented on June 11, 2024

I want to make it absolutely clear that I have no intention of making rocket science out of this. I'm asking simple questions: how do we detect problematic imports? How do we prevent them as the library develops? Give me an eslint rule and a GitHub action and you have me on board.

That which is not automated will never be followed. This has proven true in practice countless of times. I hear a solution but I don't hear how we write a "test" so the issue doesn't happen again. Naturally, such solutions are met with a bit of resistance as they are incomplete by nature.

I've also voiced this before, we are discussing both the "perfect" solution as well as the "progressive" solution (if it leads us to the perfect solution that's great). This separation and transition is important because I often miss the context in which something is proposed. Please try to explicitly mention that.

Not to mention that often the "right" solution and the "perfect" solution can be different things entirely. You can only discover this after a due discussion.

from msw.

thepassle commented on June 11, 2024

I would also add to the pros: "Increased performance"

Even the minimal usage of the library becomes verbose.

How is:

import { http } from 'msw';
import { setupWorker } from 'msw/browser';

different than:

import { http } from 'msw/http';
import { setupWorker } from 'msw/browser';

I understand that if your project also happens to use graphql you now have 3 imports instead of 2, but I don't feel thats a lot more verbose.

We can implement this on the export conditions level only. This way the source code remains in tact but provides you pointers to standalone modules.
{
"exports": {
"http": "./lib/core/http.mjs"
}
}

Im not sure what you mean with this, this is not how export conditions work. Here's an example of export conditions:

  "exports": {
+    "import": "./index-module.js",
+    "require": "./index-require.cjs"
  },

The import and require are conditions. Can you clarify what you mean with "we can implement this on the export conditions level only"?

Not all tooling understands exports conditions

I feel like we can safely disregard this, because the library has been using package exports for a long time already now

from msw.

thepassle commented on June 11, 2024

To add to the previous post:

If this was the proposal all along, then I have missed the point entirely. I've not read "export condition" once in this discussion.

This was indeed what I'm suggesting. The only entrypoints to your package are what you define in your package exports. Given that (currently) MSW defines the following keys in the package exports:
.
node
native
browser
mockServiceWorker.js
package.json

means that you can only import those things, the only valid module specifiers are:

'msw';
'msw/node';
'msw/native';
'msw/browser';
'msw/mockServiceWorker.js';
'msw/package.json';

Everything else, for example: 'msw/lib/core/http.js' will fail, because they are not allowed by the package exports as defined in the package.json. I am indeed suggesting that we open these imports up to be imported, so that we can import:

import { http } from 'msw/http';
// or
import { http } from 'msw/lib/core/http.js';
// or whatever else we decide is the best way to make these publicly available

As an aside and just to clarify, I've previously written about how package exports work here and about export conditions here

from msw.

kettanaito commented on June 11, 2024

How is: [code] different than: [code] ?

First, msw/browser and msw/node are intention-based separations. Where do I want to use MSW? In the browser. So msw/browser. They are also runtime guards because one export depends on things that the other cannot (msw/node imports node: modules that will fail in the browser). This makes is a required separation, not really an optimization thing. The fact that we get some optimization from this is a nice bonus but it was never the priority behind this decision.

Im not sure what you mean with this, this is not how export conditions work

I simplified the code for brevity. This is the actual export condition:

{
  "exports": {
    "./http": {
      "types": "./lib/core/http.d.ts",
      "default": "./lib/core/http.mjs",
      "require": "./lib/core/http.js"
    }
  }
}

Is this the suggested solution? Add these for exports like http, HttpResponse, etc? Should we also add these for all root-level exports, like getResponse? Is there somewhere we can draw the line? If so, by which criteria?

I feel like we can safely disregard this, because the library has been using package exports for a long time already now

I wish this was true but we still ship with CJS support and even with modern tooling I've seen cases where simple export conditions were not enough. Tooling still needed root-level placeholder folders like ./node/package.json.

from msw.

kettanaito commented on June 11, 2024

This was indeed what I'm suggesting

See, I misunderstood you entirely. Apologies on my part. I'm happy we are on the same page now, and the discussion helped us arrive there.

I think we can extend the exports we have. I still have some concerns about this (posted in my previous comment), as well as the educational aspect of this (when to recommend this?)

from msw.

kettanaito commented on June 11, 2024

A few more questions:

Isn't msw/node also a barrel file then? Do you propose we do msw/node/foo at some point too?
Do you have some materials on browser imports and why tree shaking is not possible? Because the browser doesn't do static code analyzis, just fetches the entire import tree? I feel if this is the future of web, it's a bit misaligned with reality. We are migrating away from the bundler to the browser but we lose a bunch of useful functionality along the way so then library authors would have to restructure their code... See, I'm likely not grasping the whole context here. I'd be thankful if you helped me improve at that.
Could you please open a pull request with minimal exports changes and share some of the performance results? It'd be nice to know what numbers we are talking about (that we aren't optimizing for 49ms). That will help us with the cost/gain conversation later on.

from msw.

thepassle commented on June 11, 2024

when to recommend this

Always 😜 (jk jk, or well, only partly joking, but I'll forfeit this point)

Is this the suggested solution? Add these for exports like http, HttpResponse, etc? Should we also add these for all root-level exports, like getResponse? Is there somewhere we can draw the line? If so, by which criteria?

Pretty much, yes, but there are several ways we can go about this, and we can discuss what would be the best way to go.

We can create entrypoints for grouped categories of things
We can use subpath exports or wildcard exports as pointed out above

As a starting point of discussion to see how we could split things up (note that this is just a suggestion and we can do lots of bikeshedding on how to actually group things):

Looking at the barrel file, we export the following:

export {
  GraphQLHandler,
  HttpHandler,
  HttpMethods,
  RequestHandler,
  SetupApi,
  bypass,
  cleanUrl,
  getResponse,
  graphql,
  http,
  matchRequestUrl,
  passthrough
};

You could consider creating a handlers.js entrypoint which would export all the handler stuff, like:

export { GraphQLHandler, HttpHandler, RequestHandler, http, graphql };

but this is problematic, because graphql pulls in a lot of modules, this is why I'd consider splitting those up into a msw/http and msw/graphql entrypoint, e.g.:
https.js

export { HttpHandler, http };

graphql.js

export { GraphQLHandler, graphql };

bypass and passthrough also seem like similar functionalities that could be grouped together, im not sure what this entrypoint should be named, perhaps something like builtins or something like that?
builtins.js

export {
  bypass, 
  passthrough 
};

Then we have:
utils.js

export {  
  cleanUrl,
  getResponse,
  matchRequestUrl
};

which seem like utils that could be grouped together, and that leaves us with:
TODO.js

export {  
  HttpMethods,
  RequestHandler,
  SetupApi
};

I'm not sure how relevant those last ones are together, maybe those should be split up differently (or maybe other things should be split up differently), but that would lead to the following added exports:

{
  "exports": {
    "./http": "./http.js",
    "./graphql": "./graphql.js",
    "./builtins": "./builtins.js",
    "./utils": "./utils.js",
    "./TODO": "./TODO.js"
  }
}

note that I've left the export conditions out for brevity, this is just to show which export keys would be added

Alternatively, we could just expose a subpath, like @mattcosta7 suggested above, or use a wildcard, like ./lib/core/*

Alternatively, maybe we dont have to create entrypoints for all of those things, maybe entrypoints for http and graphql are enough to be useful

from msw.

thepassle commented on June 11, 2024

Isn't msw/node also a barrel file then? Do you propose we do msw/node/foo at some point too?

The difference here is that msw/node only exports 2 related things, rather than it being sort of a gathering point of a bunch of (potentially unrelated) exports.

Do you have some materials on browser imports and why tree shaking is not possible? Because the browser doesn't do static code analyzis, just fetches the entire import tree? I feel if this is the future of web, it's a bit misaligned with reality. We are migrating away from the bundler to the browser but we lose a bunch of useful functionality along the way so then library authors would have to restructure their code... See, I'm likely not grasping the whole context here. I'd be thankful if you helped me improve at that.

To clarify, this is not a browser only-issue, this is how things work in Node as well. If a module imports another module, which imports 5 other modules, which import 10 other modules, you're loading all those modules. This is not a web problem exclusively.

from msw.

thepassle commented on June 11, 2024

I think we can extend the exports we have

I've created a PR here: #2004

Changes like this cannot really happen in a sustainable fashion unless there's tooling around them

I've created two packages for this, in case anybody else is reading along/is interested:

eslint-plugin-barrel-files eslint plugin to disallow barrel files and other related problems
barrel-begone Analyzes your packages entrypoints and warns about barrel files, module graph size, and other barrel-file related problems

from msw.

pleunv commented on June 11, 2024

Lowkey following this discussion because it's super interesting, and because I can tell from experience that barrel files and huge import graphs have a massively detrimental effect on the performance of tools such as Jest where tree shaking and DCE is non-existent.

I don't immediately have anything meaningful to add to the barrel discussion, but I am in the middle of migrating a huge test suite to msw and the fact that the graphql import pulls in what I expect to be almost the whole graphql-js lib in every test suite is a little worrisome. https://github.com/urql-graphql/urql for example managed to shed most of its graphql-js weight by switching to https://github.com/0no-co/graphql.web, I'm wondering if this is something you'd be willing to consider as well? Happy to help wherever.

from msw.

pleunv commented on June 11, 2024

I quickly tested this out by linking graphql to npm:graphql-web-lite@^16.6.0-3 as described here. This is only a partial measure as I suspect you still include the barrel cost since the package structure remains the same as compared to directly using https://github.com/0no-co/graphql.web, but you already shed a lot of overhead of the graphql-js package.

Taking a test file containing 30 unit tests, using msw and apollo:

Using graphql

hyperfine --warmup 3 'pnpm run test <snip>.spec.tsx'
Benchmark 1: pnpm run test <snip>.spec.tsx
  Time (mean ± σ):      9.123 s ±  0.101 s    [User: 9.508 s, System: 1.002 s]
  Range (min … max):    9.008 s …  9.261 s    10 runs

Using graphql aliased to npm:graphql-web-lite@^16.6.0-3

hyperfine --warmup 3 'pnpm run test <snip>.spec.tsx'
Benchmark 1: pnpm run test <snip>.spec.tsx
  Time (mean ± σ):      8.703 s ±  0.065 s    [User: 9.242 s, System: 0.945 s]
  Range (min … max):    8.645 s …  8.840 s    10 runs

Which in itself is already a very significant improvement. I suspect that this is also a cost you'd pay for every test file rather than every worker, but I'd have to investigate further.

edit: Turns out I'm wrong and you pay the cost only once per worker. This is 191 unit tests across 17 files running in band (1 worker):

Using graphql

hyperfine --warmup 3 'pnpm run test <snip>'
Benchmark 1: pnpm run test <snip>
  Time (mean ± σ):     26.707 s ±  0.244 s    [User: 29.486 s, System: 2.705 s]
  Range (min … max):   26.350 s … 27.173 s    10 runs

Using graphql aliased to npm:graphql-web-lite@^16.6.0-3

hyperfine --warmup 3 'pnpm run test <snip>'
Benchmark 1: pnpm run test <snip>
  Time (mean ± σ):     26.314 s ±  0.359 s    [User: 29.278 s, System: 2.569 s]
  Range (min … max):   25.937 s … 27.134 s    10 runs

One package leading to a +400ms initialization cost is rather 🤯, though.

from msw.

Avoid barrel file exports about msw HOT 44 CLOSED

Comments (44)

Proposal

The problem

Proposed solutions

Add additional entrypoints

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent