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

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

Open
4 tasks
Tracked by #501
lukekarrys opened this issue Oct 27, 2022 · 19 comments
Open
4 tasks
Tracked by #501
Labels
semver:major backwards-incompatible breaking changes
Milestone

Comments

@lukekarrys
Copy link
Contributor

lukekarrys commented Oct 27, 2022

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 Require cycle warning #398
@lukekarrys lukekarrys added the semver:major backwards-incompatible breaking changes label Oct 27, 2022
@ljharb
Copy link

ljharb commented Oct 27, 2022

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 :-/

@lukekarrys
Copy link
Contributor Author

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.

@ljharb
Copy link

ljharb commented Oct 27, 2022

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.

@lukekarrys
Copy link
Contributor Author

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

@ljharb
Copy link

ljharb commented Oct 27, 2022

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

@darcyclarke darcyclarke added this to the v8 milestone Oct 31, 2022
@darcyclarke darcyclarke mentioned this issue Oct 31, 2022
14 tasks
@donmccurdy
Copy link

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!

@ljharb
Copy link

ljharb commented Feb 14, 2023

@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".

@donmccurdy
Copy link

donmccurdy commented Feb 15, 2023

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.

@ljharb
Copy link

ljharb commented Feb 15, 2023

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

@donmccurdy
Copy link

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!

@broofa
Copy link

broofa commented Sep 23, 2023

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.

@ljharb
Copy link

ljharb commented Sep 23, 2023

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.

@broofa
Copy link

broofa commented Sep 24, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:major backwards-incompatible breaking changes
Projects
None yet
Development

No branches or pull requests

6 participants
@ljharb @broofa @darcyclarke @lukekarrys @donmccurdy and others