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

Upgrade: v7 convert to ESM lets Disscuse the API #138

Closed
frank-dspeed opened this issue Mar 21, 2022 · 6 comments · Fixed by #143
Closed

Upgrade: v7 convert to ESM lets Disscuse the API #138

frank-dspeed opened this issue Mar 21, 2022 · 6 comments · Fixed by #143

Comments

@frank-dspeed
Copy link

I have forked this repo long time ago i would love to merge back as i think there are good improvements but i found out that we need maybe to incremental adopt changes as this gets used so often.

First things first

at present we got a export of the async del version under the name del and then we assign to that the sync version this would result into really bad ESM code i would vote for renaming the named Exports in this CJS version as also in the new ESM version to deprecate the old import and require behaviors and get better static analyzeable code.

del() gets delAsync() and del.sync gets delSync() while we add a deprecated default import so that the patterns that are in the section below still work while we deprecate them.

when we transpil that code to cjs with the following rollup config we can create a del.cjs inside the node_modules folder that has the old api from v6.0.0 the only thing changes is that you require('del/del.cjs')

this pattern is the best for the future i guess i am from the NodeJS Package Maintainance Group and i was working on a Gold Standard for Packaging since then.

People will generate CJS files when they need them.

{
  input: ['del.js'],
  external: () => true, //
  output: [
    {
      preferConst: true,
      format: 'es',
      entryFileNames: `[name].cjs`,
      dir: `./node_modules/del`,
      interop: 'esModule',
      exports: 'named',
    }
  ]
}

i have the following readme snippet prepared

Deprecated Usage and How to Transpile to CJS

This usage mode resulted out of the fact that we wanted to support the old
CJS patterns from v6.0.0

When you transpile this Module With Rollup to CJS

const del = require('./del.cjs'); // deprecated version but will work
// Replaced by Async/Await DynamicImport, DynamicImport as Promise use
const { delAsync, delSync } = require('./del.cjs')
import del, { sync } from 'del';
del(patterns, delOptions); // is now delAsync { delAsync } from 'del'; delAsync();
del.sync(patterns, delOptions); // is now delSync import { delSync } from 'del'; delSync();

Short Following up

i want to do the deprecation set already in this CJS version so that we can more smoth transition to esm

so long story short can we add module.exports.delAsync and exports.delSync as also translate everything away from module.exports to directly exports also drop exports default aka module.exports and communicate to do

const { delSync, delAsync} = require('del');

if yes i could create some pull requests for example i got this fully typed with automatic typegenration working as js with JSDOC Annotations

frank-dspeed added a commit to stealify-backports/del that referenced this issue Mar 21, 2022
Introduces delAsync and delSync for later esm conversation

Compat with the old v6.0.0 API
Update now all Examples to const { delAsync, delSync } = require('del');
Leads to later import { delAsync, delSync } from 'del';
see: sindresorhus#138
This was referenced Mar 21, 2022
@frank-dspeed
Copy link
Author

frank-dspeed commented Mar 21, 2022

Review: #140 it demonstrates why i want the api change as there is no other good upgrade path at last i do not see it.

@sindresorhus
Copy link
Owner

I plan to move directly to ESM without any CommonJS support. I'm just waiting for TypeScript 4.7 (which will have full ESM support), so I don't have to deal with too many TS support issues.

The API will be two named exports: delete and deleteSync.

It's probably easier if I do the migration myself as there's a lot of details to get right.

https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

@frank-dspeed
Copy link
Author

frank-dspeed commented Mar 22, 2022

@sindresorhus i am specialist for the new ESM support in typescript 4.7 and got bad new for you and some good as long as you output a single entrypoint your package.json needs the following to be compatible to typescript moduleResolution node12 + nodenext

{
  "main": "name.js",
  "types": "name.js",
  "type": "module",
  "exports": "name.js"
}

note that you can use the .cjs and .mjs extensions with this example typescript 4.7 will then lookup: name.d.mts or name.d.cts
there is no fallback to index.d.ts!

{
  "type": "module",
  "exports": { 
    "node": { 
      "import": "./name.js",
      "types": "./name.js"
    },
    "import": "./name.js",
    "types": "./name.js"
  } 
}

the above example uses the object form and also droped types and main fild as they are not needed this will only work with moduleResolution: node12 nodenext so it is not backward compatible add main and types to stay typescript backward compatible also it shows that you need to add types for each nested level.

when you use a Object for exports a lot of edge conditions can apply this was the most basic version for a esm only module

also be aware that subPath Patterns are not supported by typescript as soon as you use them your on your own

when you want the object form you need to add a types fild to each alias and to the root alias this is WiP but the root level works and there is no option to choose the conditions that typescript will use

@jonkoops
Copy link
Contributor

jonkoops commented Jul 7, 2022

I plan to move directly to ESM without any CommonJS support. I'm just waiting for TypeScript 4.7 (which will have full ESM support), so I don't have to deal with too many TS support issues.

Does that mean you'd like to convert this library to TypeScript and compile both CommonJS and ESM?

@sindresorhus
Copy link
Owner

Does that mean you'd like to convert this library to TypeScript and compile both CommonJS and ESM?

No. The TS mention is about TS not being able to handle ESM until TS 4.7.

@jonkoops
Copy link
Contributor

I have an implementation ready for this issue under #143.

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

Successfully merging a pull request may close this issue.

3 participants