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

Breaking changes in v2.0.3 #257

Open
donmccurdy opened this issue Aug 28, 2023 · 7 comments
Open

Breaking changes in v2.0.3 #257

donmccurdy opened this issue Aug 28, 2023 · 7 comments
Assignees

Comments

@donmccurdy
Copy link
Contributor

donmccurdy commented Aug 28, 2023

Moving from v2.0.2 → v2.0.3 is a breaking change. In my own application the code below...

import type caporal from '@caporal/core';

... now throws this error:

(rpt2 plugin) Error: $PROJECT/packages/cli/src/session.ts(29,34): semantic error TS2503: Cannot find namespace 'caporal'.

That error can be fixed by doing import type * as caporal ... instead, but a number of other issues remain, related to the fact that Caporal is now being imported as ESM rather than CJS. The specifics here would depend on a user's build system, and also module resolution settings in TypeScript (if applicable).

I believe the issue is caused by introduction of the package.json#exports field. While it is technically possible to introduce that field without a breaking change, it is very difficult in practice. It's considered best practice to only add the exports field in a major release.

Perhaps it would be possible to publish v2.0.7 as a duplicate of v2.0.2, and leave the breaking changes for v3?

@mattallty
Copy link
Owner

Thanks for retorting this @donmccurdy, and sorry for the inconvenience!

Perhaps it would be possible to publish v2.0.7 as a duplicate of v2.0.2, and leave the breaking changes for v3?

yes I'm gonna do that

@mattallty
Copy link
Owner

@donmccurdy just released 2.0.7, could you please check it ?

@mattallty
Copy link
Owner

Ok seems like it's OK:

➜  ~ npm install --global @gltf-transform/cli

added 256 packages in 23s

43 packages are looking for funding
  run `npm fund` for details
➜  ~ gltf-transform --help

  gltf-transform 3.5.1 — Command-line interface (CLI) for the glTF Transform SDK.

  USAGE

    ▸ gltf-transform <command> [ARGUMENTS...] [OPTIONS...]

[...]

@mattallty
Copy link
Owner

@donmccurdy ok so 2.0.7 is now like 2.0.2
If you have some time, It would be great if you can test 3.1.5, I changed the exports and I'm curious if that's better in your case.

Change: 2f1d9ef

@donmccurdy
Copy link
Contributor Author

donmccurdy commented Aug 28, 2023

Thanks @mattallty! v2.0.7 is working as expected.

I don't have an opinion on 2f1d9ef, those changes look fine. Perhaps "import" should be renamed as "default" based on https://nodejs.org/api/packages.html#conditional-exports, but that isn't causing issues for me either way. Here's how I set up the exports, just as FYI —

https://github.com/donmccurdy/Caporal.js/blob/d67d87bfb551c33c0c94d23e4f32df1a17aa4238/package.json#L5-L14

I don't know whether that is any better or worse than what you've chosen, but I've been doing it this way in other projects for a while.

At first glance, I think your configuration on the 3.x branch is valid when a downstream ESM project uses "moduleResolution": "node" but not with "moduleResolution": "nodenext". To support the latter, your dist/* ESM builds need use file extensions on imports:

- import type { Argument } from "../types"
- import type { Command } from "../command"
+ import type { Argument } from "../types.js"
+ import type { Command } from "../command/index.js"

Perhaps it's possible to configure your build system to do this. Type definitions would need to do the same. In my own libraries I've found it easier to just switch to nodenext myself, and then the compiler will throw an error if I try to make imports that aren't compatible. This would be backward-compatible with "node" resolution.

Complete changes required would be:

Happy to open a PR for those changes if you're interested.

@mattallty
Copy link
Owner

Thanks a lot for the feedback @donmccurdy!

By reading this:

https://www.typescriptlang.org/docs/handbook/esm-node.html#new-file-extensions

New File Extensions
The type field in package.json is nice because it allows us to continue using the .ts and .js file extensions which can be convenient; however, you will occasionally need to write a file that differs from what type specifies. You might also just prefer to always be explicit.

Node.js supports two extensions to help with this: .mjs and .cjs. .mjs files are always ES modules, and .cjs files are always CommonJS modules, and there’s no way to override these.

In turn, TypeScript supports two new source file extensions: .mts and .cts. When TypeScript emits these to JavaScript files, it will emit them to .mjs and .cjs respectively.

Furthermore, TypeScript also supports two new declaration file extensions: .d.mts and .d.cts. When TypeScript generates declaration files for .mts and .cts, their corresponding extensions will be .d.mts and .d.cts.

Using these extensions is entirely optional, but will often be useful even if you choose not to use them as part of your primary workflow.

and your link:

"import" - matches when the package is loaded via import or import(), or via any top-level import or resolve operation by the ECMAScript module loader. Applies regardless of the module format of the target file. Always mutually exclusive with "require".
"require" - matches when the package is loaded via require(). The referenced file should be loadable with require() although the condition matches regardless of the module format of the target file. Expected formats include CommonJS, JSON, and native addons but not ES modules as require() doesn't support them. Always mutually exclusive with "import".
"default" - the generic fallback that always matches. Can be a CommonJS or ES module file. This condition should always come last.

... I'm also wondering if I could also play with file extensions (cjs and mjs) to let Node & TS guess the exact format, but that's just my understanding, I still have to dig deeper.

Anyway, thanks for the help!

@donmccurdy
Copy link
Contributor Author

... I'm also wondering if I could also play with file extensions (cjs and mjs) to let Node & TS guess the exact format ...

By Node's interpretation .js == .mjs if you have "type": "module" in package.json. I believe it's OK to use both .cjs and .mjs explicitly too! My build tool just happens to always choose .js for whichever build matches package.json#type, and .cjs or .mjs for builds that don't match.

I think it's just the implicit file extensions that are preventing v3.x from working in my builds now. This includes imports pointing to external dependencies lodash/filter.js.

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

No branches or pull requests

2 participants