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

publish both cjs and esm version #105

Closed
wants to merge 1 commit into from
Closed

Conversation

jiby-aurum
Copy link
Collaborator

@jiby-aurum jiby-aurum commented Sep 3, 2022

Fixes issue with vite, where mui and tss-react imports different versions of @emotion/react, one being cjs and the other being esm.

@garronej garronej mentioned this pull request Sep 4, 2022
@garronej
Copy link
Owner

garronej commented Sep 4, 2022

Hey @jiby-aurum,
Thanks for the PR!
Unfortunately I can't publish as-is (I have released a beta though) because pepoles import with import {} from "tss-react/mui" in your PR it becomes import {} from "tss-react/dist/<esm | cjs >/mui".
I'm working toward resolving the issue but first I would like to be able to reproduce it.

@jiby-aurum
Copy link
Collaborator Author

@garronej I can update the pr, such that tss-react/mui is also exported, was not aware this was a public api, I will update the pr in a while

@garronej
Copy link
Owner

garronej commented Sep 4, 2022

@jiby-aurum in any case, thank you very much for your involvement.
You are greatly helping the project by helping me sort out the this issue.
I am realy surprised no one complained before...

@garronej
Copy link
Owner

garronej commented Sep 4, 2022

I can reproduce

@garronej
Copy link
Owner

garronej commented Sep 4, 2022

Don't worry about updating the PR.
I understand now, I'll do it myself now.

@jiby-aurum
Copy link
Collaborator Author

@garronej perfect!

It should rather be me thanking you for the awesome project, would have been a pain to migrate away from makeStyles to upgrade mui. Thanks a lot for the quick turnaround :)

@garronej
Copy link
Owner

garronej commented Sep 4, 2022

You are welcome!

Everything is working runtimewise but I still get some red when trying to import the submodule... 🤔

image

@jiby-aurum
Copy link
Collaborator Author

@garronej I believe you will have to define the types inside exports for each entry (https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing)

@garronej
Copy link
Owner

garronej commented Sep 4, 2022

@jiby-aurum you 🪨

@garronej
Copy link
Owner

garronej commented Sep 4, 2022

I would have bet it would work but it doesn't for some reason 🤔

image

@garronej
Copy link
Owner

garronej commented Sep 4, 2022

image

@jiby-aurum
Copy link
Collaborator Author

@garronej typescript ‼️ maybe try forcing the resolution via typesVersions (https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#version-selection-with-typesversions), something like

"typesVersions": {
  "*": {
     "*": ["./dist/esm/*"]
  }
},

@jiby-aurum
Copy link
Collaborator Author

jiby-aurum commented Sep 4, 2022

@garronej btw you were correct about assuming it will work, it does work if the consuming project has moduleResolution set to nodenext or node16, seemingly the package.json exports support only in these resolution modes, this was in https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing if you scroll up a bit, I did not notice as well.

but I think it makes more sense with the typesVersions hack as then it works for everyone irrespective of what they have for moduleResolution

@jiby-aurum
Copy link
Collaborator Author

@garronej just tried 4.1.0-beta.3, works for me perfectly

@jiby-aurum
Copy link
Collaborator Author

@garronej sadly no, the normal imports complain now 😢

@garronej
Copy link
Owner

garronej commented Sep 4, 2022

@jiby-aurum I'm still tweeking things

@jiby-aurum
Copy link
Collaborator Author

jiby-aurum commented Sep 4, 2022

@garronej I have a proposal, which is maybe the least friction change from current stable release.

  1. we keep the cjs build as it is. (It goes directly into dist and your denoify module copies it to root level)
  2. We then build the esm version into an esm folder, (it would go into dist/esm and would be copied to esm folder by denoify)
  3. We add just the following to package.json
{
  "module": "dist/esm/index.js",
  "exports": {
    ".": {
      "import": "./dist/esm/index.js",
      "require": "./dist/index.js"
    },
    "./mui": {
      "import": "./dist/esm/mui.js",
      "require": "./dist/mui.js"
    }
  },
}

Im assuming denoify will update all these paths, to remove dist.

I like this for the reasons that

  1. We are not touching at all how cjs assets work
  2. we don't need any hack for typescript as types are all there in the root

@garronej
Copy link
Owner

garronej commented Sep 4, 2022

Again thank you for helping me on this, I am strugling.

Im assuming denoify will update all these paths, to remove dist.

You assume correctly yes, gg for figuring that out by yourself.

I like this for the reasons that

Bummer, I was getting hyped, I want to try some more to make it work..
Plus, this feature that I just discovered make me want to do something like:

image

(it dosen't work though)
(mui.ts and mui_compat.ts as identical runtypewise, juste the types def aren't the same)

It would be so cool not to have to tell users that use older TS version to import the module diferently.

And we still have the problem of the default import not working, still trying to figure out how typesVersions realy works...

image

@jiby-aurum
Copy link
Collaborator Author

@garronej I will push my proposal in this branch. And yes typesVersion is just compile time, aka just types

@jiby-aurum
Copy link
Collaborator Author

jiby-aurum commented Sep 4, 2022

@garronej this branch now has my proposal, I tried it locally and it works properly in my project setup

for exports and module I had to put the path without dist as denoify_enable_short_npm_import_path seemingly does not update them

@garronej
Copy link
Owner

garronej commented Sep 4, 2022

@garronej this branch now has my proposal, I tried it locally and it works properly in my project setup

Thank you!

for exports and module I had to put the path without dist as denoify_enable_short_npm_import_path seemingly does not update them

Yes, I'm updating the script right now.

"!dist/tsconfig.tsbuildinfo",
"!dist/esm/test/",
"!dist/esm/bin/",
"!dist/esm/package.json",
Copy link
Owner

Choose a reason for hiding this comment

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

🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well, I just copied what was there for the cjs currently, did not really check if these files are generated

Comment on lines +12 to +20
"exports": {
".": {
"import": "./esm/index.js",
"require": "./index.js"
},
"./mui": {
"import": "./esm/mui.js",
"require": "./mui.js"
}
Copy link
Owner

Choose a reason for hiding this comment

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

Ok but we haven't the types that automatically resolve to compat.d.ts and mui_compat.d.ts with older typescript version...

Copy link
Owner

Choose a reason for hiding this comment

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

Does this make sence to you?

image

I mean do you think i'm on the right path with this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@garronej it makes sense if you want to use the compact types when typescript in consuming project is less than 4.4, but keep in mind that as you mentioned earlier this is only compile time, only the types changes, the implementation won't be the compact ones.

garronej added a commit to garronej/denoify that referenced this pull request Sep 4, 2022
@garronej
Copy link
Owner

garronej commented Sep 4, 2022

@jiby-aurum Let's forget about using typesVersions to resolve to the compat declaration file for now.
Ama release 4.1.0 thank you again for your help.

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.

2 participants