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

Is lazy-cache really necessary? #39

Closed
alecmev opened this issue Dec 5, 2015 · 5 comments
Closed

Is lazy-cache really necessary? #39

alecmev opened this issue Dec 5, 2015 · 5 comments

Comments

@alecmev
Copy link

alecmev commented Dec 5, 2015

I don't think it's a good idea to wrap require in any way, ever. Many libraries / tools are built on the assumption that it remains untouched, because it's pretty much a language construct by now (being an ES5 equivalent of import).

I've seen the Browserify trick in the lazy-cache readme, but it isn't implemented in markdown-toc, and it's not really a solution, but a hack. I want to use this in my client-side code, which involves putting everything through Webpack, and this is the only issue preventing me from having a warning-free build:

WARNING in ./~/markdown-toc/lib/insert.js
Critical dependencies:
4:33-40 require function is used in a way in which dependencies cannot be statically extracted
@ ./~/markdown-toc/lib/insert.js 4:33-40

I'm not a language expert, so I might be wrong and this is a valid use of require, and I should be looking into just suppressing Webpack instead, but, FWIW, I haven't seen any package do this before (i.e. cause warnings like the one above).

@jonschlinkert
Copy link
Owner

Sounds like this just needs to be updated to the latest lazy-cache format.

Is lazy-cache really necessary?

Is webpack necessary? You could hand-code all of that yourself, but that's silly. What you're really asking is: "Is it really necessary to try to get the best possible performance from your modules?". To which my answer is, "No, but I'll try my hardest, and I'll base my decisions on facts, data and experience."

I don't think it's a good idea to wrap require in any way, ever.

If wrapping require bothers you, then you probably shouldn't look at what browserify and webpack do to your code.

it's not really a solution, but a hack.

Isn't that semantics? Without the hack neither webpack nor browserify can find lazy-requires, with the hack ~~~they~~~ browserify can.

This hack has turned out to be a pretty good solution. Instead of thinking that lazy-evaluation is a bad solution because the tools you use fail when it's used, perhaps you should be looking for ways to solve that problem. I do, because I know how big the benefits are.

lazy-cache has had something like 8m downloads in the past 4 months, with two issues - including this one, both related to webpack. Materially, both issues were on projects that use the old-style lazy-cache, and we've had zero actual issues on lazy-cache itself. browserify does not have this issue.

IMO this is something that can be solved in a build step, or with a webpack plugin.

@alecmev
Copy link
Author

alecmev commented Dec 6, 2015

I'm okay with Browserify / Webpack doing crazy things to my code, because that's expected of them. They're pretty much transpilers, making require / import work in browsers.

This, however, is a regular library. Lazy loading does make sense here, but, in the end, on the client-side, where it matters the most, gray-matter still ends up being in the same bundle as markdown-toc (with Browserify; correct me if I'm wrong).

Webpack has its own mechanism for on-demand loading - require.ensure - but it doesn't work anywhere else (well, it did, but seems like nobody cared enough about server-side lazy-loading to maintain it; can be shimmed, with the benefits gone). lazy-cache is admittedly more convenient, but, on top of breaking the convention of having imported modules as variables, it also works only in Node, and nowhere else (it has no benefits in Browserify).

You're saying that this issue should be resolved in Webpack, but why should lazy-cache be preferred over the several-years-old require.ensure?

Sorry for challenging your design choices like this, I don't mind being proven wrong.

@jonschlinkert
Copy link
Owner

You're saying that this issue should be resolved in Webpack

Well, I'm saying it's "meet in the middle". We can use the hack that got this working for browserify, and we could create a plugin for webpack to convert the lazy-requires to standard requires. I don't know webpack, it seems like it shouldn't be too difficult.

I get your point about how the benefits of lazy-evaluation are lost in non-node usage, but this is, after all, a node.js package. But again, I'd like to find a solution that allows us to take advantage of lazy-caching for node.js, but also make it work with browserify and webpack. I think the community in general would/should see great benefit in that

@alecmev
Copy link
Author

alecmev commented Dec 7, 2015

Out of curiosity, what was the reason for not doing something like this?

if (file) {
  return require('gray-matter').stringify(res, file.data);
}

Lazy-loading + caching in Node, while working out of the box in Browserify / Webpack / some other lib we could be unaware of.

Just in case, I'm using this library as a client-side patch for some missing backend functionality, which isn't going into a public release, so there's no urgent need for a fix.

@doowb
Copy link
Collaborator

doowb commented Dec 20, 2015

@jeremejevs try using the unlazy loader in your webpack build.

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

No branches or pull requests

3 participants