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

Transpiling all @scoped packages in Yarn Workspaces project? #143

Closed
mrtnzlml opened this issue Dec 3, 2020 · 21 comments
Closed

Transpiling all @scoped packages in Yarn Workspaces project? #143

mrtnzlml opened this issue Dec 3, 2020 · 21 comments
Labels

Comments

@mrtnzlml
Copy link

mrtnzlml commented Dec 3, 2020

Hello! 👋

Is your feature request related to a problem? Please describe.

Up until version 5, we were using this project to transpile our internal dependencies inside Yarn Workspaces project. Something like this:

withTranspileModules(['@adeira'])

However, starting from version 5, seems like we have to be explicit about all our dependencies:

withTranspileModules([
  '@adeira/css-colors',
  '@adeira/fetch',
  // ... many more rows
  '@adeira/sx',
  '@adeira/test-utils',
])

This, unfortunately, includes even deeply nested dependencies which is not only confusing but also hard to maintain. You can see what I mean in this PR: https://github.com/adeira/universe/pull/1444/files

What we really want is to transpile any @adeira package inside the monorepo without even thinking about it. Arguably, we might have been using this package for something it was not intended for. Is it true? How should we deal with this situation?

Describe the solution you'd like

The original behavior in version 4 was great!

withTranspileModules(['@adeira'])

Or something like this allowing us to comfortably work with Yarn Workspaces with many interlinked packages.

Describe alternatives you've considered

Fork this project in version 4 and keep expanding on the previous behavior with different goals.

Additional context

none

@martpie
Copy link
Owner

martpie commented Dec 3, 2020

This is a consequence of the new module resolution. @adeira not being an actual module, it cannot be transpiled.

I understand that it was quite convenient, imho explicitness is a good thing (this plugin is nothing but a big hack). If this gets enough traction/attention we may implement a workaround, but nothing's planned right now, modules need to be explicitly listed.

Hope you understand 👍

@martpie martpie added the 6.x label Dec 3, 2020
@mrtnzlml
Copy link
Author

mrtnzlml commented Dec 5, 2020

In case someone else is going to have the same question - my colleague workarounded it like this: adeira/universe#1462 (essentially automatically resolving the local workspace dependencies and sub-dependencies).

Still, it would be nice to consider this use-case (automatically transpile our local workspaces, nothing else). 🙂

@belgattitude
Copy link

belgattitude commented Dec 9, 2020

I understand that it was quite convenient, imho explicitness is a good thing (this plugin is nothing but a big hack). If this gets enough traction/attention we may implement a workaround, but nothing's planned right now, modules need to be explicitly listed.

@martpie @mrtnzlml I'm in favour of keeping it explicit.

Myself I don't always use next-transpile-modules... (but most of the time yes 😄 )

When applicable I tend to use the typescript resolutions support initiated in vercel/next.js#13542, and for that explicitness is required.

BTW, congrats for version 6 !

PS: A comparison exists here https://github.com/belgattitude/vercel-monorepo-test/tree/master (not trying to do advertisment)

@martpie
Copy link
Owner

martpie commented Dec 9, 2020

@belgattitude Aside: I really like this idea of separating apps and packages, it makes the clear distinction between "consumers" and "comsumed" modules.

I'm also in favor of keeping that explicit for the version listed above, rather than black magic 😄

I will close this for now, as said before, if there is a lot of traction, I may reconsider it.

ps: @mrtnzlml thank you for the link shared, it's a really interesting implementation. It would work for yarn workspaces, but not all kind of setup, like modules coming from npm etc. It might be useful to other people though!

@martpie martpie closed this as completed Dec 9, 2020
@mrtnzlml
Copy link
Author

mrtnzlml commented Dec 9, 2020

I understand what you are both saying. Yet, let me offer a contra-argument why I think the explicitness is not a good thing in our scenario. It's simply because it's very fragile and hard to maintain manually. Why? Because you have to specify not only the direct dependencies but also dependencies of dependencies and their dependencies.

So for example in this PR, Michal is adding murmur-hash dependency? But why? You won't find any mention of it in the whole Next.js project. Well, it's because it's a deep dependency of some other one. What if we add another local dependency into murmur-hash? We silently break all projects using next-transpile-modules again. 😫

Being explicit on this level might work on a small project where you can imagine the whole graph of local dependencies but not in our case when I have no clue what depends on what unless I dig deep every time. So we have these choices:

  • add all @adeira/* dependencies explicitly, check their @adeira/* dependencies and also add them explicitly, check their @adeira/* dependencies and also add them …, OR
  • add all of them explicitly without even thinking if it's needed
  • add them as we break things uncovering the hidden/deep dependencies
  • 🤔

All of the choices are fragile because it will break down the road unless we keep the list up to date somehow.

That being said, again, we might be using this package for something it was not intended for. We really don't need to transpile any NPM module, no. Instead, all we need to is to transpile all the local Yarn workspaces just like Babel does.

Hope this point of view is understandable as well. 🙂

@martpie
Copy link
Owner

martpie commented Dec 9, 2020

The dependency of a dependency is known (and a separate issue imho).

Let's assume I install a library X, and the library X depends on Y, which uses fat arrow functions. With TM, the build will probably pass, but crash on IE 11 (happened on a professional project a couple of weeks ago).

Now, who is responsible? In my case, the maintainer or Y explicitly said: "I'm not supporting IE11, deal with it". Well, in that case you will need to require('next-transpile-modules')(['Y']) 🤷‍♂️

It's a little bit different than your problem right here, but just really similar (just at another higher level). It does not make sense to require('next-transpile-modules')(['X']) here, as X itself does not need to be transpiled).

You also run into other problems, which are "if A and B depends on C, and B is explicitly transpiled", and with your system, we automatically transpile all dependencies of transpiled modules, should I transpile A as well? And even if no, it means next-transpile-modules need to check all dependencies of B, and add them to transpiled modules (could be hundred of deps) which would be terrible for perfs, and would add a lot of false positives - deps that would not need to be transpiled, but are).

Now your scenario is simpler than all those edgecases, you just want to transpile a scope, but if tomorrow I add this feature, I need to support everyone's setup, and every setup is unique in its own way (babel, ts paths, workspaces, pnp, npm, pnpm, lerna...). It's just too much work, and out-of-scope of what this plugin is trying to solve.

So I think this is the responsibility of the developers to do what you guys did, which is "get all the modules under that scope, and pass them to withTM".

edit: what I'm clumsily trying to say is there is a difference between yarn's workspaces and scopes: do we want to transpile automatically a scope? or do we want to just transpile "all the packages of my workspace" (so, tight to Yarn especially)


sidenote: for deps of deps (like @adeira/murmur-hash here), if you install it in node_modules/@adeira/the-module-using-murmur-hash/node_modules/@adeira/murmur-hash, then you should be fine by just transpile @adeira/the-module-using-murmur-hash.

I have no idea if it's possible to tell Yarn "do not hoist that pls", or if it's even a good idea, but this would work).


@andrekorolev
Copy link

I was struggling with this deps of deps compiling issue whole week, since I migrated to NTM:6.3.0. I fully understand your idea about zones of responsibility for package owners and package users - explicitness in that meaning is a good term and it makes sence. But in practice when I try to find the root package, which fails in IE, I loose hope.
I added "main" field in all my packages, then I listed all dependencies that I found in next.config.js, but still es6 code get in bundle. It is really hard to find, where it comes. And it is just simpler to roll back to NTM:4.1.0

@martpie maybe you know the way how to find packages in compiled Next bundle, which were not compiled?

@martpie
Copy link
Owner

martpie commented Feb 9, 2021

My best recommendation to debug that is to do the following:

  • add config.optimization.minimize = false; to you Next.js config
  • run a production build
  • run it on IE
  • open the console, jump to the line where it failed
  • goes a little bit up in the lines of code, and check the Webpack comment telling you which module is affected

Hope that helps!

@andrekorolev
Copy link

andrekorolev commented Feb 9, 2021

Thanks a lot!
It seems that this is a problem of yarn and how it manages workspaces. Yarn detects packages of different versions and put them in sub-directories if needed. Thus, when I tell NTM to transpile some package, it takes it from root node_modules level, but not from nested node_modules directories. As a result, I have a mix of different packages' versions, and some of them are not transpiled.
Now I'm searching for differences and trying to align them.

@martpie
Copy link
Owner

martpie commented Feb 9, 2021

That's indeed the way to go. Try to see where you have duplicated dependencies and examine why, then fix the problem at the source.

50% of the problems people face with this plugin are because of misunderstanding of how their setup work (yarn workspaces for example). There may be a couple of points about that in the FAQ that may interest you.

@belgattitude
Copy link

belgattitude commented Feb 9, 2021

@andrekorolev don't know if it's helpful, some tools that I use.

es-check

Not perfect, but I use in a github action just as safety belt for our ie11 requirements. A simple yarn add es-check --dev to install and for convenience, you can add it to the script section and run it on the dist/build folder

    scripts: {
       "es-check": "es-check es5 './.next/static/chunks/**/*.js' -v",
    }

When it fails, I try to locate the package from the error message (having sourcemaps makes the proces easier but not mandatory).

I add the culprit to the next-transpile-modules config and try again.

If it's not a direct dependency, I first locate it by running

yarn why <package_name>

optional: if I see there's a lot of semver compatible versions, I generally use npx yarn-deduplicate yarn.lock at the root of the monorepo, it's always good to do if it fits / this can be done in a hook and enforced in a github action. The lock file still contains duplicates in some circumstances)

An option is to move the transitive dep into a direct dep...

And try once more.

When you get used to it it's pretty straightforward. But not all monorepos are equally made.

By the way... NPM 7 has a recently introduced support for workspaces, but I still recommend to use yarn (v1) to handle monorepos.

If you're looking for an example, I've set up this https://github.com/belgattitude/nextjs-monorepo-example to play, maybe you'll find out some good material for you.

Next-transpile-modules in my experience is a very helpful plugin. Does the job super well, congrats to @martpie !

All the best.

@belgattitude
Copy link

belgattitude commented Feb 9, 2021

One more note

Yarn detects packages of different versions and put them in sub-directories if needed

Flattening directories can only be done when semver is guaranteed. The yarn-deduplicate is helpful, but you can always force the resolution in the

// package.json
{
   resolve: {
     'an-external-package-locked-for-everyone': '5.1.1'
   }
}

This has a cost in term of maintenance, cause now you must really take care of breaking updates.

Just saying it's possible, not necessarily a good idea 😄

@belgattitude
Copy link

belgattitude commented Feb 9, 2021

which fails in IE, I loose hope.

@andrekorolev

I feel the pain... But don't worry, in my experience you're on the good direction with next-transpile-modules and yarn... When you'll get past the es6 problem, there will be just adjustments with some missing polyfills if needed and all will work smooth.

@martpie
Copy link
Owner

martpie commented Feb 9, 2021

Sidenote: to add a little on what @belgattitude said: IE11 is always annoying to support, but should be part of your projects requirements/stories/planning.

On all my projects, I have an IE11 task/epic that I estimate between 3 to 5 days, so this is perfectly normal and is part of our job (and we all hate it!).

One day we won't have to support it anymore, and this day will be celebrated till the end of times.

@belgattitude
Copy link

Hey @martpie , btw I've seen a guy who did a script based on es-check (helps readability). Haven't tested it but nice to know

vercel/next.js#10440 (comment)

@rodrigo-arias
Copy link

rodrigo-arias commented Apr 1, 2021

Just leaving a note here for you to reconsider transpiling the modules into a @scope. Currently I'm building a component library with Lerna and it becomes very complicated and fragile to manually declare all the individual packages to be transpiled. I understand all the points against compiling workspaces, but it would be nice to have that feature for all packages in a scopes. Thanks.

@martpie
Copy link
Owner

martpie commented Apr 2, 2021

Hello @rodrigo-arias,

I don't plan to work on this anytime soon. That said, the "only" thing you need is to retrieve the name of all the packages under a scope. And pass it to ntm.

Maybe there is a package for that, or you can write your own script to retrieve all those package names.

Something like

require('next-transpile-modules')(getScopePackageNames('@myscope'))

I will pin the issue for visibility, and maybe if there is more traction on this I will reconsider it.

Also, PRs are welcome if you manage to create a helper for that, and then we can discuss how much if we want it in the package, or just in the FAQ.

Good luck!

@rodrigo-arias
Copy link

@martpie that makes sense to me. Thanks for your quick response!

@martpie martpie pinned this issue Apr 2, 2021
@nandorojo
Copy link

Here's my solution, which I mentioned on #149:

// next.config.js

const path = require('path')
const fs = require('fs')

// you might need to change this to the relative path of your node_modules!
const node_modules = path.resolve(__dirname, '../..', 'node_modules')

const scopes = [
  '@fullcalendar', 
  '@motify',
  '@nandorojo',
  '@dripsy', 
]

const scopedPackages = []

fs.readdirSync(node_modules)
  .filter((name) => scopes.includes(name))
  .forEach((scope) => {
    fs.readdirSync(path.resolve(node_modules, scope))
      .filter((name) => !name.startsWith('.'))
      .forEach((folderName) => {
        const { name, ignoreTranspileModules } = require(path.resolve(
          node_modules,
          scope,
          folderName,
          'package.json'
        ))
        if (!ignoreTranspileModules) {
          scopedPackages.push(name)
        }
      })
  })

// add it to transpile modules
module.exports = require('next-transpile-modules')(
  [
    'dripsy',
    'expo-next-react-navigation',
    'moti',
    ...scopedPackages,
  ],
  { resolveSymlinks: true }
)

@gaurav5430
Copy link

We were using this for transpiling some of our node module dependencies, using a scoped name, similar to the original author.

In our case, it might be ok to just read the names of these scoped packages from package.json and pass on to this library as we don't have any nested dependencies setup right now, but would really like to have a better, more maintainable solution in the long term.

Is there an alternate library that provides that functionality?

@martpie
Copy link
Owner

martpie commented Mar 4, 2022

FYI, A workaround was added to the README by @MostafaNawara :)

@martpie martpie unpinned this issue Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants