Skip to content

Conversation

@brettz9
Copy link

@brettz9 brettz9 commented Jun 1, 2019

  • Breaking change: Node files now in dist folder (as UMD)
  • Breaking change: No more default (module.exports) exports for keys.js, is_arguments.js (to avoid ambiguity with named exports)
  • Enhancement: Add ES6 distribution and point to it from module in package.json
  • Refactoring: ESM in source
  • Git: Add ignore file
  • npm: Add npm-recommended package-lock.json
  • npm: Add recommended engines, bugs, homepage, contributors, dependencies
  • Testing: Use non-deprecated Buffer.from
  • Linting: Add rc file with recommended settings; add ignore file

While I added Babel as part of the Rollup pipeline, I can either remove it, or take advantage of it and convert the source to more modern ES code, but for this PR, I thought I'd keep things simple and only change the source as far as ES6 Modules.

Your code is still widely used, and I think it would be helpful for some pipelines to be able to avoid the need for CommonJS conversion. I would not expect the "breaking changes" to impact any normal uses of your code (and I've changed main to point to the dist UMD file).

- Breaking change: No more default (`module.exports`) exports for `keys.js`, `is_arguments.js` (to avoid ambiguity with named exports) - Enhancement: Add ES6 distribution and point to it from `module` in `package.json` - Refactoring: ESM in source - Git: Add ignore file - npm: Add npm-recommended `package-lock.json` - npm: Add recommended engines, bugs, homepage, contributors, dependencies - Testing: Use non-deprecated `Buffer.from` - Linting: Add rc file with recommended settings; add ignore file
@ljharb
Copy link
Member

ljharb commented Jul 29, 2019

Thanks!

However, CJS is fine; there's virtually zero value in breaking changes for these purposes.

A dist is not necessary at all.

Also, only apps should have lockfiles, never published things.

@ljharb ljharb closed this Jul 29, 2019
@brettz9
Copy link
Author

brettz9 commented Jul 30, 2019

CJS is not fine for projects wishing to use ESM out of the box, but I get the hesitance to add potentially breaking changes.

Re: lock files, you might take a look at https://yarnpkg.com/blog/2016/11/24/lockfiles-for-all/

@ljharb
Copy link
Member

ljharb commented Jul 30, 2019

node itself doesn’t support ESM out of the box, and some form of bundling will always be required for the browser.

When/if node does support dual ESM/CJS packages, and only then, will it be appropriate to add ESM (not remove or replace cjs) to npm packages.

I’m familiar with that post, and i disagree with it. Having a dev-only lockfile in your package artificially insulates project developers from breakage that their users can and do experience - it’s hostile to your users. When a transitive dep breaks, your package’s tests should suddenly start breaking - and when it’s fixed they should suddenly start working again - that’s the point of how semver works.

@brettz9
Copy link
Author

brettz9 commented Jul 30, 2019

Re: Node, sure--though one can already take advantage of experimental support. However, bundling is not always required in the case of browser demos which directly point to node_modules paths. Despite pitfalls to watch for with path resolutions, directly importing from node_modules in the browser is a convenient way to allow one's tests or demos/samples to work without a build step and reduce the duration of the feedback loop even compared to watch routines which take some time to complete a build (not to mention this approach avoids any bugs or complexities with source map resolution). I use this approach across a number of projects, and it has been quite convenient.

Re: breaking changes, a burden is both put on new users who come across these bugs. semver allows the enforcement of a basic system of trust, but it does not insist on a particular usage pattern. There are advantages for such trust being at the pace of consuming libraries which can use tools like npm-check-updates to do mass updates at their leisure, and try them out before exposing all of their users to the latest updates. semver is still convenient in such cases as it tends to minimize the burden on consuming libraries which can choose, for example, to use their semver patterns to upgrade only theoretically non-breaking changes but with the ability to verify through their own testing that there indeed are no new breaking changes. In such cases, semver still should protect them from having to adjust for many breaking changes at once and avoid exposing any of their users to unexpected changes.

@ljharb
Copy link
Member

ljharb commented Jul 30, 2019

"without a build step" isn't something that should be optimized for, imo.

For apps, I agree with you - a lockfile is necessary. The issue is that a dev-only lockfile, like yarn.lock or package-lock, protects the least important cohort (maintainers) and leaves the most important cohort (users) exposed to breakage, which creates a false sense of safety/security/correctness - which is thus more harmful than no protection at all.

@brettz9
Copy link
Author

brettz9 commented Jul 30, 2019

In most packages I work with, the maintainers end up being the most important cohort because there are few to no regular dependencies, with devDependencies such as Rollup and Babel determining what goes into the distributed files. With the maintainers as the guardians of what gets released (and it makes sense for the onus to be on them so they can vouch for what gets exposed to users, with their tests serving as a solid contract). You also seem to expect a build step as the norm (as do I, though, as mentioned, I feel it is worthwhile for maintainers to directly use modules so as not to be deterred from frequent or in-depth experimenting with long build steps).

This is not to mention that users who go to the trouble of noticing the presence of a lock file are more likely to know enough about them to know they will not work when transitively required.

There is also the practical benefit that developers may be alerted to their own failure to update and incorporate updated devDeps if they notice they have updates to package.json devDeps. without an updated lock file also showing up in the repo diffs.

@ljharb
Copy link
Member

ljharb commented Jul 30, 2019

Anything that relies on "if they notice" isn't going to get noticed most of the time.

@brettz9
Copy link
Author

brettz9 commented Jul 30, 2019

It is better than not having the chance to be noticed at all. If devDeps fail to be updated in the scenario I described, there will be no harm, as it should still be passing all tests and not impacting downstream users.

@ljharb
Copy link
Member

ljharb commented Jul 30, 2019

Sure, if there was a way to have package-lock only apply to devDeps then I'd immediately add that to all 1-200 packages I maintain - but there's not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants