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

fix: add export field into package.json #982

Closed
wants to merge 8 commits into from
Closed

Conversation

danielbarion
Copy link
Member

@danielbarion danielbarion commented Mar 10, 2023

  • use not minified files into package.json
  • continue exporting minified files because the community can import those files directly from dist if they want
  • add an export field into package.json

To try this PR just install this beta release:

yarn add react-tooltip@5.10.1-beta.5

or

npm install react-tooltip@5.10.1-beta.5

image

close #976
close #981

@danielbarion danielbarion marked this pull request as ready for review March 10, 2023 16:53
package.json Outdated Show resolved Hide resolved
@danielbarion
Copy link
Member Author

yarn add react-tooltip@5.10.1-beta.6

or

npm install react-tooltip@5.10.1-beta.6

@danielbarion danielbarion requested review from GerkinDev and gabrieljablonski and removed request for GerkinDev March 12, 2023 03:25
package.json Outdated Show resolved Hide resolved
Co-authored-by: Jaco <jacogr@gmail.com>
@danielbarion
Copy link
Member Author

yarn add react-tooltip@5.10.1-beta.7

or

npm install react-tooltip@5.10.1-beta.7

cc @gabrieljablonski @GerkinDev @jacogr

@danielbarion danielbarion requested review from jacogr and GerkinDev and removed request for GerkinDev and jacogr March 12, 2023 15:36
@GerkinDev
Copy link
Contributor

I'll try it on my bug repro, and check if webpack (the oldest bundler in the place) behave correctly. Maybe later today

@GerkinDev
Copy link
Contributor

image
#976 seems to work now
I tested a create-react-app and it worked fine too.

But #972 fails again using react-tooltip@5.10.1-beta-7. Digging in

Copy link
Contributor

@GerkinDev GerkinDev left a comment

Choose a reason for hiding this comment

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

#972 regression

@GerkinDev
Copy link
Contributor

GerkinDev commented Mar 12, 2023

See #972 (comment) for investigation results. They seem to indicate that pointing to non-minified version is not a reliable solution to the issue.

@danielbarion
Copy link
Member Author

danielbarion commented Mar 13, 2023

yarn add react-tooltip@5.10.1-beta-8

or

npm install react-tooltip@5.10.1-beta-8

@GerkinDev this beta release is pointing to minified files

@GerkinDev
Copy link
Contributor

GerkinDev commented Mar 13, 2023

This time also, works with both minified & unminified (I modified the package.json of installed version) with swcMinify: false, but does not work with both minified & unminified with swcMinify: true or unset.

By setting a breakpoint in the repro app, somehow, swc reassigns the react import to a number....
See the value of a:
1st call:
image
2nd call:
image

@danielbarion danielbarion changed the title fix: use not minified files into package and add export field fix: add export field into package.json Mar 13, 2023
@GerkinDev
Copy link
Contributor

GerkinDev commented Mar 13, 2023

Honestly I don't know how to reliably fix the issue. Given that some builds seems to work, while some other don't, only with swc, I have the feeling that it's somewhat related to a bug in the mangling map of swc. The shortest reliable solution I've found is swcMinify: false. Yet, I understand this is not a satisfying one

I've tracked down the faulty reassignation from within the bundled @floating-ui/core that somehow assign a number to the react import after minification. Awkward ? Yes it is, that's why I think it's a bug on THEIR side. It might be worth to try again with @floating-ui marked external, so that swc handles module linking itself. But I could not guarantee this would be a reliable solution even if it works.

That kind of issue is a hell.

@danielbarion
Copy link
Member Author

@GerkinDev can you confirm if the bug is happening on 5.8.3? Maybe the solution is we back to rollup as a default bundler.

5.8.3 - Rollup - ~ 14kb (final files sizes)
5.9.0 - Esbuild - ~ 9kb (final files sizes)

PR: #959

@GerkinDev
Copy link
Contributor

I'll explore that. Also, I'll do tests with other versions of next.js, maybe it's a regression in a dependency or it might have been patched in the meantime.

@jeyang
Copy link

jeyang commented Mar 13, 2023

Confirming that it's working for me in 5.8.3

@danielbarion
Copy link
Member Author

@jeyang can you test this beta release and let me know if it's working, please?

yarn add react-tooltip@5.10.1-beta-9

or

npm install react-tooltip@5.10.1-beta-9

This beta version was built with rollup instead of esbuild

@jeyang
Copy link

jeyang commented Mar 14, 2023

for react-tooltip@5.10.1-beta-9 I'm getting this error message:

./node_modules/react-tooltip/dist/react-tooltip.min.mjs
Can't import the named export 'createContext' from non EcmaScript module (only default export is available)

image

@danielbarion
Copy link
Member Author

danielbarion commented Mar 22, 2023

@jeyang can you test if the beta release with rollup works for you, please? (another PR)

#984 (comment)

edit: or can you provide a reproducible example, so I can test, please?

@consense
Copy link

consense commented Mar 22, 2023

same issue for me. It comes up when using storybook (version @storybook/react": "6.5.16)

Using normal webpack build:

  • 5.8: works fine
  • 5.10: works fine
  • 5.10.1-beta-9: works fine

Using storybook

  • 5.8: works fine
  • 5.10: Error: Module parse failed: Unexpected token
  • 5.10.1-beta-9: ModuleDependencyError: Can't import the named export 'createContext' from non EcmaScript module (only default export is available)

@danielbarion
Copy link
Member Author

Please check my comment at: #984 (comment)

@danielbarion danielbarion deleted the fix/next-13-swc branch March 30, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants