node-semver icon indicating copy to clipboard operation
node-semver copied to clipboard

[ENHANCEMENT] remove individual file requires as part of API contract

Open lukekarrys opened this issue 3 years ago • 19 comments

It will always be possible to directly require a file, but as part of the next major version I would like to no longer consider that part of the public API. When setup properly, tooling exists to do much better tree-shaking than existed when the current directory structure was created.

  • [ ] Move all files to lib/
  • [ ] implement pkg json exports
  • [ ] Audit names and paths of exports
    • do we still want the functions/ prefix, etc
  • [ ] Try to remove cycles #398

lukekarrys avatar Oct 27 '22 03:10 lukekarrys

oof, why not? Granular requires / deep imports are a superior way to access items, and it obviates the need for treeshaking (which can never fully achieve what “granular importing” can, due to the nature of JS). If anything, I’d prefer to see the api move to only separate files :-/

ljharb avatar Oct 27 '22 03:10 ljharb

It has made doing some refactoring in this repo difficult since moving any file is a breaking change. For example, I'm pretty sure the require cycle can be avoided by shuffling some stuff around. I might be wrong on that specific case, but I think the point still stands.

I'm also open to keeping the file-centric API and only using a major release to move files around. I'm much more interested in being able to move some files around once during the next major release, than removing that API entirely.

lukekarrys avatar Oct 27 '22 03:10 lukekarrys

That can be mitigated by adding “exports”, which is itself a breaking change, but could still preserve the per-file requires, while allowing any changes on non-entrypoint files as a semver-patch.

ljharb avatar Oct 27 '22 03:10 ljharb

Updated the original comment to use the exports field. I do think that's the ideal solution, especially for function heavy API like semver.

lukekarrys avatar Oct 27 '22 07:10 lukekarrys

Awesome - more than happy to help review the exports field syntax to ensure desired compatibility.

ljharb avatar Oct 27 '22 16:10 ljharb

Would solving this issue also mean providing ES Module exports, or is that a separate thing? Not sure if that's implied by moving to exports field syntax. Thanks!

donmccurdy avatar Feb 14 '23 22:02 donmccurdy

@donmccurdy ESM can import CJS just fine, so i'm not sure what there is to solve there - the module format wouldn't necessarily change as part of adding "exports".

ljharb avatar Feb 14 '23 22:02 ljharb

Thanks @ljharb! I was running into a build error that manifested as a semver-related message...

import { clean, gte, valid, lt } from 'semver';
         ^^^^^
SyntaxError: Named export 'clean' not found. The requested module 'semver' is a CommonJS
module, which may not support all module.exports as named exports. CommonJS modules can
always be imported via the default export, for example using:

import pkg from 'semver';
const { clean, gte, valid, lt } = pkg;

... and thought this might be related. The error disappeared after a clean rebuild though, so I think something in my project's build got out of sync during a branch change. No issues now, and thanks for the quick reply.

donmccurdy avatar Feb 15 '23 14:02 donmccurdy

That’s an issue with your bundler, almost never with the package. CJS works perfectly in anything that supports node modules.

ljharb avatar Feb 15 '23 15:02 ljharb

Yes, agreed, it turned out to be an issue in the build state and/or bundler.

CJS works perfectly in anything that supports node modules.

As someone maintaining open source libraries that need to run in multiple platforms, multiple module formats, and with multiple bundlers, I wish it were that simple... 😓

In any case, the semver package is not the issue, sorry for the off-topic aside!

donmccurdy avatar Feb 15 '23 15:02 donmccurdy

FWIW, I landed here hoping to see this module had adopted an ESM-only stance, or at least had an package.json#exports field in the works.

deep imports are a superior way to access items

I'm not sure I agree. Deep-imports require a lot of manual effort to manage. Module authors have to structure the code in a way that's conducive to deep importing. Any changes to the file structure will break existing imports. And users have to manually manage each and every import. That gets pretty onerous once you have more than 2-3 of them.

broofa avatar Sep 23 '23 16:09 broofa

It's true that proper architecture requires manual effort to manage. As for "structure the code", you just have to put stuff in separate files, which shouldn't be a burden since it's already common practice.

Any changes to the file structure will break existing imports.

With the "exports" field, that's not the case, because the entry points simply don't inherently correspond 1:1 to the filesystem. You can move your files around willy-nilly as long as the entry points in "exports" remain.

And users have to manually manage each and every import. That gets pretty onerous once you have more than 2-3 of them.

vscode does this pretty automatically for many years now, and even prior to that, I've managed dozens of imports in a file quite easily, so I don't understand this claim at all.

ljharb avatar Sep 23 '23 21:09 ljharb

you just have to put stuff in separate files, which shouldn't be a burden since it's already common practice

I'm not sure it's actually common practice. And, if it is, it's not because it's a natural way of structuring code. It's because having one-export-per-file is the only way of allowing deep-imports to achieve comparable code-removal characteristics to tree shaking. (You're gonna have a hard time convincing me that developers actually want to structure their code as a  bunch  of  1-2 line  files.)

With the "exports" field

This package doesn't provide an exports field, though... at least not yet. Until it does, I think my points still apply. E.g. I just switched to deep semver imports in npmgraph. Because of the lack of exports, VSCode did nothing for me other than complain when I mistyped the file paths. (I acknowledge this is a separate issue from ESM vs. CJS, however.)

Anyhow... I don't mean to start an argument. More just hoping to "+1" the notion that the old CJS-style of module construction is kind of a nuisance.

broofa avatar Sep 24 '23 01:09 broofa

Is that can we fixed end set on one stable file separate

LuisLopez44431 avatar Apr 23 '24 17:04 LuisLopez44431

Ok if we set a small protect end we keeping busy some reliable

LuisLopez44431 avatar Apr 23 '24 17:04 LuisLopez44431

End stay in sinta

LuisLopez44431 avatar Apr 23 '24 17:04 LuisLopez44431

End we keep on syntax

LuisLopez44431 avatar Apr 23 '24 17:04 LuisLopez44431

Well how bout in GitHub end we set end advisory of what is the specialist the file

LuisLopez44431 avatar Apr 23 '24 17:04 LuisLopez44431

Well let's check what's is happening in CJS end we find out

LuisLopez44431 avatar Apr 23 '24 17:04 LuisLopez44431