martpie / next-transpile-modules

Next.js plugin to transpile code from node_modules. Please see: https://github.com/martpie/next-transpile-modules/issues/291
MIT License
1.13k stars 85 forks source link

Watching doesn't work on top level imports #9

Closed masad-frost closed 1 year ago

masad-frost commented 5 years ago

I understand that watching seems to work because it works in the sample repo linked here https://github.com/martpie/next-plugin-transpile-modules/issues/5#issuecomment-442516611

The thing is that if you import from a top level file in the package i.e. /shared/index.js watching doesn't work on that file. It's really odd, that is the case with yarn workspaces and file linking.

See https://github.com/masad-frost/monorepo-typescript-next-the-sane-way/compare/a5d89d844eba37231586f85564d7d47d433cdd04...master updating the contents of shared/index (index can be anything by the way) doesn't trigger a recompilation, it has to be an import from a subfolder of the module

masad-frost commented 5 years ago

The issue doesn't seem to be related to the regex either :/. Interesting problem

masad-frost commented 5 years ago

Not sure if what I'm doing is wrong but setting resolve.symlinks back to true seems to fix it.

martpie commented 5 years ago

Interesting, I will try to reproduce it. Thank you for the report!

Big up for the repro!

martpie commented 5 years ago

I think the problem comes from your code in this module.

Hot reloading works well when having modules in this file:

const tada = () => {
  return 8
};

export default tada;

pages/index.tsx

import * as React from 'react';
import * as Calc from 'shared/utils/calc';
import tada from 'shared/index';

export default () => {
  return (
    <>
      <div>Hello World</div>
      <div>5 + 3 = {Calc.add(5, 3)}</div>
      <div>{tada()}</div>
    </>
  );
};

Hot reloading works well on my side when doing import shared from 'shared'; or import shared from 'shared/index';

martpie commented 5 years ago

And here's a screencast (click on it for HD):

screen recording 2019-01-29 at 09 49 17

with

➜  website git:(master) ✗ node -v
v8.11.3
➜  website git:(master) ✗ npm -v
6.5.0
masad-frost commented 5 years ago

Weird doesntwork

I'm not sure what I'm doing wrong. Also tried with yarn file: and yarn workspaces. shared/utils or anything that's nested in a subfolder is watched

Edit:

Also ran in docker under chakracore-8.11.1 image

npm install cd website npm install npm run dev change file no changes

masad-frost commented 5 years ago

Sorry for the slow gif

martpie commented 5 years ago

~Could it be linked with Chakracore? Can you reproduce it with a classic Node.js setup?~

Sorry, I misread your message.

What is your version of node, npm and what OS do you use?

masad-frost commented 5 years ago

Ubuntu 18.04 node 9.11.1 npm 6.5.0

masad-frost commented 5 years ago

I first ran into this issue under docker on node:10.7.0 image with yarn workspaces

martpie commented 5 years ago

Are you using yarn workspaces or npm install to install your local dependencies?

martpie commented 5 years ago

Could reproduce it on Linux. Investigating.

martpie commented 5 years ago

References:

Can't do much for now, I'll check again why I enabled the resolve.symlinks rule, it seems to be working without, but I know I had trouble without it before.

masad-frost commented 5 years ago

Nice. Thanks man, will keep an eye out and report back if I face issues with symlinks off

martpie commented 5 years ago

if I face issues with symlinks off

Yes please! I am investigating on my side as well, did not face any error so far, but I know I added it for a reason I can't remember ><

NathanielHill commented 5 years ago

I'm having similar issue on Ubuntu.

I'm using yarn workspaces with the packages in the root of the monorepo.

This is my only config: transpileModules: ['@app'] and I'm importing from @app/package.

I have to restart next to see changes in my shared components.

martpie commented 5 years ago

@NathanielHill Did you try to disable symlinks and see what happens?

NathanielHill commented 5 years ago

I thought that was disabled by default :thinking:

https://github.com/martpie/next-transpile-modules/blob/ca563f4720db1e2e0818bb927f64e9f7bef3532c/index.js#L30

NathanielHill commented 5 years ago

Just for the heck of this, I changed that to true and instantly ran into problems.

@martpie Am I not understanding what you're asking?

martpie commented 5 years ago

Just for the heck of this, I changed that to true and instantly ran into problems

thank you, this is what I meant :) I will investigate more on this issue this week-end. A guy from Webpack asked to provide a reproduction of this bug.

NathanielHill commented 5 years ago

Well, it's pretty obvious why.

If I'm importing '@app/components/foo' and I turn resolve.symlinks to true...

Then it will (in my case) try to import '../components/foo' which it will find, but that path won't match the RegEx used by this plugin.

So it doesn't get passed through babel and fails due to syntax errors.

NathanielHill commented 5 years ago

@martpie Do we know if this is working on other platforms (with symlinks = false)?

Sounds like maybe this is a webpack problem with linux file watching?

NathanielHill commented 5 years ago

I have not gotten watching working for any shared code (outside my next dir).

iwarner commented 5 years ago

I have the same issue on my Mac now - just seem to arrive recently when I upgraded nextJS also

martpie commented 5 years ago

@iwarner I did not reproduce it on macOS, can you provide a repro?

iwarner commented 5 years ago

Ok just tested some stuff:

NextJS 7.0.3 "@weco/next-plugin-transpile-modules": "^0.0.2",

-- This works √

Next 7.0.0 -> 7.0.3 next-transpile-modules - v1 + v2

This does not work unfortunately

The Ideal combination I know is Next 7.0.3 and your v1 - but the watch is broken somehow

martpie commented 5 years ago

Yes, but hot reloading probably does not work. Can you provide a repro so I can try to fix it?

iwarner commented 5 years ago

Sorry yes the hot reloading works with NextJS 7.0.3 "@weco/next-plugin-transpile-modules": "^0.0.2",

-- This works √

-- Unfortunately I cannot provide a Repo on this

For clarity the hot reloading is linked directly to the watching right?

otroboe commented 5 years ago

@NathanielHill Any news? I have a pretty similar issue.

I have a monorepo, and I'm doing a yarn link to a lib outside my monorepo. When I don't use this plugin, the watch is working, but when I use it, I need to restart the nexjs app.

martpie commented 5 years ago

Hello guys, I have a lot on my plate right now, but I will try to work on it, it's a bit hard to debug as my development machine is a Mac, but I'll do my best. I have a couple of ideas I would like to try.

Having a reproduction repo would help a lot.

otroboe commented 5 years ago

I have this issue at my job, and I don't have time right now to make a repo I'm sorry :-( It's not blocking the team, and it's not a priority either, sadly.

RafalFilipek commented 5 years ago

Hi!

@martpie I think you can use official with-yarn-workspaces example from next.js repo. In this case config.resolve.symlinks = true solution works just fine.

Right now I have similar issue, but with TypeScript. If you're using ts / tsx files you can't use config.resolve.symlinks = true because you will get error about missing webpack loader for ts/tsx file.

After few trys I came with quite simple solution (no idea if it’s correct, but it works).

My project

/packages/
  /portal   // @app/portal
  /ui       // @app/ui
  /icons    // @app/iconcs
withTranspileModules({
  transpileModules: ["ui", "icons"],
  webpack(config) {
    config.resolve.symlinks = true;
    return config;
  }
});

As you can see the biggest change is that I'm not providing package names but folder names.

Hope it helps :)

martpie commented 5 years ago

this config.resolve.symlinks is apparently a problem for many. I could probably make it opt-out in a future release. Would that solve everyone's problem?

RafalFilipek commented 5 years ago

Not sure if this is a 'solid' solution :) I think we need 3 repos:

Each repo must be checked on MacOS / Windows / Linux with two configurations: symlinks: true / false.

Yesterday I checked the solution from the previous comment on Windows. It worked without a problem. But I'm not sure about this part:

As you can see the biggest change is that I'm not providing package names but folder names.

martpie commented 5 years ago

@RafalFilipek I think your problem is unrelated and described by the FAQ: https://github.com/martpie/next-transpile-modules#i-have-trouble-with-transpilation-and-flowtypescript

RafalFilipek commented 5 years ago

@martpie I have babel.config.js :) I'll make simple repo with example.

otroboe commented 5 years ago

Hello! I created a demo repository for my situation with yarn workspaces: https://github.com/otroboe/next-transpile-modules-demo

Basically a "main app" with a shared lib. Not even a specific babel configuration. When I change the code in the lib, my app doesn't reload, I have to restart it to have the change.

I hope it can help you, and maybe others!

I'm on Ubuntu 18.04.

milesrichardson commented 4 years ago

This is still a problem for me, but only for files in the top level of the package, as others have mentioned.

martpie commented 4 years ago

I will try to jump again on the problem and see if it's fixable or if we need to wait for Webpack 5.

Any help is welcome

milesrichardson commented 4 years ago

It's a really weird bug, but personally it's not too big a of a problem, because I can just put the code in a dist/ or src/ folder or whatever.

I'm not sure the root cause. The regex looks okay to me (tested it on regex101.com with some inputs) but I'm not super familiar with how webpack regex matching works. The fact that the top-level is missing makes me think something is "eating" it, which makes me suspect the regex... but not sure

merrywhether commented 4 years ago

It seems you want symlinks: false only if you are trying to have this package transpile your local packages. If you just need this for transpiling random node modules, then you don't seem to need this change. In our case, we're transpiling our local packages via their own process, so symlinks: true is necessary for Next to pickup our rebuilt packages and hot reload.

It seems like since the simple case is transpiling some random es6/esnext package from npm, the side effect of changing the webpack config doesn't need to be the norm. Whereas people who are working in more complicated repos would be more likely to understand why they might need to opt in to this behavior, especially when combined with a README entry along the lines of "if you're using this to transpile local packages in a workspace/etc, set the config option...".

Either way though, a config flag for controlling this would be great as it would avoid having to add a random webpack config change into the main body of an app's webpack config.

martpie commented 4 years ago

I will think about adding a configuration option to change this. I may have time to work on this this week-end :)

Sidenode @merrywhether, if you are transpiling npm packages, what is the problem with watching? You're not changing those files yourself 🤔

merrywhether commented 4 years ago

Oh, we have a workspaces/lerna monorepo with lots of packages consumed by a few Next applications. Each package has its own build:watch capability that we use during local dev, so we only need this module for transpiling public modules that have es6 features for those apps whose support matrix forces es5-only. For the apps that needed such transpiling, setting symlinks: false broke Next's ability to "see" changes as our packages rebuilt and thus broke HMR for package work. So in our case, this module produced an odd side-effect for untargeted modules.

We explicitly want the Next apps to use the output from the packages' own build process (though it's a mostly-identical babel setup to what the apps do) since we also publish these packages and want to ensure we're working against the same output as any other consumer.

merrywhether commented 4 years ago

@martpie Thanks for adding the resolveSymlinks config option. Works great, and I can remove the custom override now.

crossjs commented 3 years ago

resolveSymlinks lead to Module parse failed: The keyword 'interface' is reserved with typescript files

masad-frost commented 3 years ago

Yikes, I don't have that problem with TS

fabb commented 3 years ago

Is there a difference between resolveSymlinks and followSymlinks? In webpack 4 i used to do this workaround to get HMR working again, now i'm planning to update to webpack 5, but only read about resolveSymlinks everywhere

"prepare": "rexreplace \"followSymlinks: false\" \"followSymlinks: true\" ./node_modules/watchpack/lib/DirectoryWatcher.js -V"

EDIT: yes, Fast Refresh works with the default settings, I did not have to set followSymlinks anywhere.