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

next-transpile-modules is deprecated and I believe it's slowing down the site #294

Closed
FelipeEmos opened this issue Dec 31, 2022 · 2 comments

Comments

@FelipeEmos
Copy link

FelipeEmos commented Dec 31, 2022

Hi, @nandorojo, thanks for the amazing lib!
Recently I've been playing with it and exploring my options for a next project.

Can you please go through some of my questions?
If it's the case I'll maybe be able to open a PR and contribute a little.

First of all, I tried adding a lib that required SSR ( Server Side Rendering ) and had some troubles with that.
Later I found you were in the same situation as I in this discussion.

Then it clicked, I had to add the lib to the group of next-transpile-modules and it worked great.
But as you said, that's kind of a necessary evil, as it doesn't help with performance.

Question 1

I didn't understand why the app is being transpiled

const withTM = require('next-transpile-modules')([
'solito',
'dripsy',
'@dripsy/core',
'moti',
'app',
])

To test my hypothesis, I removed it and it works perfectly. It even squeezed some performance points in the Chrome's Lighthouse reports that I did.

Is there a reason it's there that I'm not seeing? Can I remove it and open a PR?
I believe it's just a matter of removing it from the 4 examples.... right?

Question 2

The README.md file has a section about adding dependencies.
Should we add an observation about this transpilation workaround for SSR? I can do that.
It's pretty likely someone will try adding a dependency ( probably UI related ) and stumble on the same problem in the future.

Question 3

Are you aware that this lib (next-transpile-modules) was deprecated?
Here's a cute good bye post from the lib maintainer: https://github.com/martpie/next-transpile-modules/releases

From now on that's a native feature from Next.JS
Unfortunately that doesn't exist in the current version of next we're using, so an upgrade is necessary.
Is there any special reason we're in such version of next? If not, I could try upgrading it, let's hope things don't break that easily, it's Javascript we're talking about here 🙃

@nandorojo
Copy link
Owner

I didn't understand why the app is being transpiled

I don't see how this will work if it's not. It's a third-party package that ships in typescript files as far as Next.js is concerned. You're saying it works for you without that?

Should we add an observation about this transpilation workaround for SSR? I can do that.

I wouldn't want to do this without better understanding what's going on here. Transpilation shouldn't have an impact on SSR. What in particular about transpiling affects the output? It might just be that transpilePackages from Next.js is better optimized for Next than next-transpile-modules was.

Are you aware that this lib (next-transpile-modules) was deprecated?

Yes, just waiting to make sure the community confirms Next.js 13.1 is satisfactory for this.

@FelipeEmos
Copy link
Author

FelipeEmos commented Jan 5, 2023

I didn't understand why the app is being transpiled

I don't see how this will work if it's not. It's a third-party package that ships in typescript files as far as Next.js is concerned. You're saying it works for you without that?

Yes, it worked for me without the app in the next-transpile-module list.

  • Maybe removing it adds problems I didn't observe because of my quick observation (in a very simple project)?
  • Maybe it was a cache thing on my end that made it work?

I don't know, but I've just educated myself a little better about the need of the next-tranpile-module and you're right.
We should keep the app in the list at least following a theoretical standpoint of what the next-transpile-module is supposed to do. It's a third party package outside of the next root, we have to bring it in ( with the tranpilation ).

Sorry for bothering if I did.

I'm closing the issue then 🙂

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