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

feat: esm and cjs #280

Closed
wants to merge 34 commits into from
Closed

feat: esm and cjs #280

wants to merge 34 commits into from

Conversation

mshanemc
Copy link
Contributor

@mshanemc mshanemc commented Oct 4, 2023

DO NOT MERGE

exploration of dual-publish for libraries
@W-14154855@

Design

cjs publishes to /cjs, esm to /esm. Using exports breaks projects who imported from kit/lib anyway (see cjs QA PR below) so no value in keeping the ambiguous lib

the compile step in wireit now runs the 2 compile jobs.

  • esm runs from the main tsconfig
  • there's a separate tsconfig-cjs which extends (overwrites in our case) the esm tsconfig.

Repo-specific Issues

lodash

lodash was "vendored" in from a custom build. It caused a lot of problems because of its default exports. Now it imports the various lodash functions needed for external. This let us get rid of the TS wrappers, the build step, etc. To avoid the cost of a larger node_modules folder (all of lodash)...

BREAKING CHANGE: lodash methods are no longer re-exported. If you need lodash function, import them yourself!

Notes for dual-publishing other repos

  1. relative/local imports need to end in .js. I'd like to have an eslint rule to migrate all these
  2. see the pjson changes around type, exports, files
  3. having to write a special pjson to the compiled folder was recommended. sfdx-core definitely fails without it. I'd consider putting these in dev-scripts so that it's easy to call them in all the libraries. I tried writing json as part of the script instead of copying from the fs, but couldn't get it to work right on gha/pwsh.
  4. be sure to use the sfdevrc changes--changing wireit on pjson won't stick, otherwise. I think if I had to "do all the libraries" I'd have dev-scripts manage these wireit variations instead of sfdevrc.json
  5. gitignore requires ignoring the esm and 'cjs' folders, similar to lib
  6. mocha runs with "node-option": ["es-module-specifier-resolution=node", "loader=ts-node/esm"], via mocharc
  7. esModuleInterop was necessary because of lodash, otherwise TypeError: (0 , lodash_tonumber_1.default) is not a function at Env.getNumber (/Users/shane.mclaughlin/eng/kit/src/env.ts:239:27) FWIW, I saw similar behavior in sfdx-core around some other modules with export = (jszip). This might cascade the requirement for esModuleInterop across the whole codebase
  8. there is a lodash-es module and an option on lodash-cli to export es modules. They worked ok, but they caused problems for sfdx-core to understand. (Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/shane.mclaughlin/eng/kit/node_modules/lodash-es/tonumber.js from /Users/shane.mclaughlin/eng/kit/lib/env.js not supported. Instead change the require of tonumber.js in /Users/shane.mclaughlin/eng/kit/lib/env.js to a dynamic import() which is available in all CommonJS modules.) I made some other adjustments to sfdx-core after going back to regular lodash, so maybe that would have eventually worked. 🤷🏻
  9. top-level js files need to be cjs (.eslintrc, commitlint, etc)
  10. there was at least one place where sfdx-core had to change because the types didn't match (findKeys). That's because kit's typing didn't match lodash's. Writing custom type wrappers for a library sounds bad.

Testing

package available @salesforce/kit@3.0.14-qa.0

  • (this makes sure that what goes to npm is valid...that my pjson.files is correct)

cjs QA via forcedotcom/sfdx-core#950.

this pr has the breaking changes updates (ex: direct imports of lodash, etc)

esm QA

use the ESM version of oclif/plugin-update oclif/plugin-update#643
add kit as a depedendency and then start using it in the code.

for both, it should compile and tests should pass. Also check the dev experience--that you get types, jsdoc, etc

@mshanemc mshanemc marked this pull request as ready for review October 9, 2023 22:39
Copy link
Contributor

@mdonnalley mdonnalley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA:

  • Added @salesforce/kit@3.0.14-qa.1 to @oclif/plugin-test-cjs-1 and @oclif/plugin-test-esm-1
  • Added this code to a command in each plugin:
import {Duration, sleep, env, parseJson, ensureArray} from '@salesforce/kit'
await sleep(Duration.seconds(1).milliseconds)
console.log(env.getString('PATH'))
console.log(parseJson('{"foo":"bar"}'))
console.log(ensureArray('foo'))
console.log(ensureArray(['foo']))

✅ Code compiled and ran in cjs plugin
✅ Code compiled and ran in esm plugin

@mshanemc
Copy link
Contributor Author

there's parts of this that are probably valid (undoing lodash, etc) but the esm thing seems less useful than leaving that alone for now.

was a good POC to see what it would take

@mshanemc mshanemc closed this Jun 10, 2024
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 this pull request may close these issues.

3 participants