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

Source Maps in node_modules package #8071

Closed
JeremyGrieshop opened this issue Dec 5, 2019 · 34 comments · Fixed by #8227
Closed

Source Maps in node_modules package #8071

JeremyGrieshop opened this issue Dec 5, 2019 · 34 comments · Fixed by #8227

Comments

@JeremyGrieshop
Copy link

JeremyGrieshop commented Dec 5, 2019

I haven't had much luck being able to debug/set breakpoints in source compiled with create-scripts and thought my use case may be somewhat unique, so I wanted to ask it here. If it's something I should take up with VSCode, I will, but I needed to start somewhere.

My setup is VS Code with an app created by create-react-app. I can debug it just fine, and I hit my breakpoints, as expected, so source maps are indeed working. However, I recently moved some of our components out into its own libraries, so they can be published separately and used by multiple apps. So, now they look like (I've simplified the example):

app/
  package.json
  src/
    App.js

libA/
  components/
    package.json
    src/
      componentA.js
    lib/
      componentA.js
      componentA.js.map

Now, I can only set breakpoints in lib/componentA.js instead of src/componentA.js (pre-compiled source). Is this something that I should be able to get working?

Any direction would be helpful. Thanks in advance!

Edit: perhaps it's related to #2355 ?

@justingrant
Copy link
Contributor

@JeremyGrieshop - Are you running the just-released CRA/react-scripts 3.3 or 3.2? 3.3 has a fix (#7022 -- full disclosure it's my PR) that is a partial fix for sourcemap issues in CRA apps. Specifically, it fixes the case where node_modules dependencies don't have sourcemaps.

For node_modules dependencies that do have sourcemaps, there's more fixing needed. #2355 is on the right track, but the current source-map-loader package is apparently not maintained and unfortunately it has serious bugs which are addressed in this fork: https://github.com/Volune/source-map-loader/tree/fixes which I've been using (via customize-cra) for 6+ months with great results.

I've been planning a follow-up PR to #7022 which will fix #2355, but first I wanted to get some questions answered from sourcemap experts about how to handle relative paths from one node_modules dependency to another node_modules dependency.

Also before adding source-map-loader to CRA, there needs to be a long-term supported home of source-map-loader because although @Volune's fork works well, it's not officially maintained either. Luckily it's only one relatively short file so hopefully finding a home won't be too hard. It may be short enough for the CRA maintainers to consider including it in CRA, but that seems like a last resort.

@JeremyGrieshop
Copy link
Author

@JeremyGrieshop - Are you running the just-released CRA/react-scripts 3.3 or 3.2? 3.3 has a fix (#7022 -- full disclosure it's my PR) that is a partial fix for sourcemap issues in CRA apps. Specifically, it fixes the case where node_modules dependencies don't have sourcemaps.

I have not yet. I literally saw that release and note about 20 minutes after I posted this. I plan to try it out this weekend and find out, though!

For node_modules dependencies that do have sourcemaps, there's more fixing needed.

Unfortunately, this is the boat I'm currently in. My node_module packages are my own that I generate source maps for, via direct babel compile.

#2355 is on the right track, but the current source-map-loader package is apparently not maintained and unfortunately it has serious bugs which are addressed in this fork: https://github.com/Volune/source-map-loader/tree/fixes which I've been using (via customize-cra) for 6+ months with great results.

I've been planning a follow-up PR to #7022 which will fix #2355, but first I wanted to get some questions answered from sourcemap experts about how to handle relative paths from one node_modules dependency to another node_modules dependency.

I'm very curious about your findings here, as I also have the unique situation where my node_modules package also has a dependency on another of my node_modules packages, which in turn has its own source maps!

Also before adding source-map-loader to CRA, there needs to be a long-term supported home of source-map-loader because although @Volune's fork works well, it's not officially maintained either. Luckily it's only one relatively short file so hopefully finding a home won't be too hard. It may be short enough for the CRA maintainers to consider including it in CRA, but that seems like a last resort.

I'll be happy to try anything out if you need feedback or testing. Thanks for the reply!

@justingrant
Copy link
Contributor

@JeremyGrieshop - thanks for the offer of help! I think at this point there are two blocking issues to getting this fixed:

  1. source-map-loader seems to be unmaintained and its master branch is too buggy currently for use in CRA. Someone would need to take ownership of that code and merge a few PRs that have been pending for 1+ years, or would have to incorporate its ~150-ish lines of code into CRA or some other library. https://github.com/Volune/source-map-loader/blob/fixes/index.js is the code of the patch version that I'm using-- as you can see it's not much code, but still someone has to commit to maintaining it going fwd.
  2. Even in @Volune's patch branch linked above, there's still a remaining blocking issue: how to handle sourcemaps of node_modules packages that in turn link to other node_modules packages. I'm discussing this question with @loganfsmyth here: https://babeljs.slack.com/archives/C6CMS6M6U if you want to lurk or discuss. Given that this is a relatively unusual case that affects relatively few npm packages, this issue could be deferred until later, but given that getting changes into CRA seems to take 6-12+ months, I was hoping to resolve it as part of the PR to include source-map-loader.

If you can help resolve either of these issues, your help would be very welcome!

@justingrant
Copy link
Contributor

Also, if you're looking for a workaround in the meantime, I use customize-cra to inject source-map-loader into my CRA config. Below is a simplified version of the config-overrides.js file that I've been using for the last few months.

const { override, useEslintRc, addWebpackModuleRule, addWebpackPlugin } = require('customize-cra');
const { produce } = require('immer');

const updateWebpackModuleRules = config => {
  const rules = config.module.rules;
  if (rules.length !== 3) {
    throw new Error('Unexpected CRA config. Exiting.');
  }

  const newConfig = produce(config, cfg => {
    const sourceMapLoader = {
      enforce: 'pre',
      exclude: /@babel(?:\/|\\{1,2})runtime/,
      test: /\.(js|mjs|jsx|ts|tsx|css)$/,
      use: 'source-map-loader',
    };
    const rules = cfg.module.rules;
    rules.splice(1, 0, sourceMapLoader);
  });

  return newConfig;
};

const overrides = [updateWebpackModuleRules];
module.exports = override(...overrides);

If you're not familiar with customize-cra, here's how to use the code above:

  1. save it into config-overrides.js in your project's root folder
  2. install the following packages:
    npm i -D github:volune/source-map-loader#fixes immer customize-cra react-app-rewired
  3. Update your package.json scripts to use react-app-rewired instead of react-scripts. Like this:
    "start": "react-app-rewired start",
    "build": "react-app-rewired build",
    "test": "react-app-rewired test",

@JeremyGrieshop
Copy link
Author

@justingrant This workaround absolutely worked without any effort - thank you!!!!

I can understand the longer-term dilemma of making it supportable by CRA. If everyone dumped all of their wishlist into CRA, it would probably look a lot like, well, webpack. As the React ecosystem grows, however, the trend of creating react component libraries will likely become more common place and I suspect more people will want the functionality of debugging them from larger projects / monorepo architecture. Even more so now that VS Code is growing in its user base. I hope to see something like this included out of the box.

Again, if there's anything I can do to help, I'd be happy to do so in a way that I can. Thanks again for your help here, this will save me countless hours of console.log debugging lol.

@middiu
Copy link

middiu commented Dec 17, 2019

Hi @justingrant,

is there a particular reason why you use volune/source-map-loader and not the normal source-map-loader module?

@justingrant
Copy link
Contributor

Yep. The current release of source-map-loader only supports absolute paths in a sourcemap's sources array but libraries are supposed to use relative paths. So it's kinda useless without fixing the relative-paths bug.

You can read more details in @Volune's PR here: webpack-contrib/source-map-loader#75

Unfortunately, source-map-loader is essentially unmaintained at this point-- no checkins since Aug 2018. Lots of PR's queued up, but no maintainer seems to be home. I (and others) have tried to ping maintainers but so far no response.

@middiu
Copy link

middiu commented Dec 17, 2019

Yep. The current release of source-map-loader only supports absolute paths in a sourcemap's sources array but libraries are supposed to use relative paths. So it's kinda useless without fixing the relative-paths bug.

You can read more details in @Volune's PR here: webpack-contrib/source-map-loader#75

Unfortunately, source-map-loader is essentially unmaintained at this point-- no checkins since Aug 2018. Lots of PR's queued up, but no maintainer seems to be home. I (and others) have tried to ping maintainers but so far no response.

thanks mate, pretty useful.

@middiu
Copy link

middiu commented Dec 22, 2019

@justingrant is there a way to get it working using the customize-cra api addWebpackModuleRule?

I'm trying to use this configuration but I keep getting errors:

const { override, useEslintRc, addWebpackModuleRule, addWebpackPlugin } = require('customize-cra');

const sourceMapLoader = {
    enforce: 'pre',
    exclude: /@babel(?:\/|\\{1,2})runtime/,
    test: /\.(js|mjs|jsx|ts|tsx|css)$/,
    use: require.resolve('source-map-loader'),
  };

module.exports =  override(
      addWebpackModuleRule(sourceMapLoader)
    )

and this is the error:

image

@justingrant
Copy link
Contributor

@middiu - If I remember correctly, addWebpackModuleRule didn't work for this case because source-map-loader needs to be injected in the middle of the CRA webpack rules (because the sourcemaps it emits need to be used as input to the Babel step that comes later), but addWebpackModuleRule only appends rules to the end. That said it's been a while since I got this working so my memory may be fuzzy. The code sample I posted above should work, however.

@middiu
Copy link

middiu commented Dec 22, 2019

thanks again @justingrant.

Your snippet of code was not working for me, but I managed to get it working in this way:

const {
    override,
    overrideDevServer
  } = require("customize-cra");
  
  const fs = require("fs");
  
  const sourceMapLoader = {
    enforce: "pre",
    exclude: /@babel(?:\/|\\{1,2})runtime/,
    test: /\.(js|mjs|jsx|ts|tsx|css)$/,
    use: "source-map-loader"
  };

  const updateWebpackModuleRules = () => config => {
    const newConfig = {...config};
    newConfig.module.rules.push(sourceMapLoader);
    return newConfig;
  };

  const devServerConfig = () => config => {
      console.log(config);
    return {
      ...config,
      https: {
        key: fs.readFileSync(process.env.REACT_HTTPS_KEY, "utf8"),
        cert: fs.readFileSync(process.env.REACT_HTTPS_CERT, "utf8")
      }
    };
  };
  
  module.exports = {
      webpack: override( 
          updateWebpackModuleRules()
      ),
      devServer: overrideDevServer(
          devServerConfig()
      ),
  }

of course the part around devServer and HTTPS is irrelevant for this issue.

Thanks a lot

@justingrant
Copy link
Contributor

OK everyone, I opened PR #8227 to add source-map-loader to CRA. Please add your feedback and/or votes!

@stale
Copy link

stale bot commented Jan 24, 2020

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jan 24, 2020
@justingrant
Copy link
Contributor

Not stale. Just waiting for a maintainer to review #8227 which should fix this issue.

@stale stale bot removed the stale label Jan 24, 2020
@stale
Copy link

stale bot commented Feb 23, 2020

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Feb 23, 2020
@justingrant
Copy link
Contributor

Not stale. Just waiting for a maintainer to review #8227 which should fix this issue.

@stale stale bot removed the stale label Feb 23, 2020
@stale
Copy link

stale bot commented Mar 24, 2020

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Mar 24, 2020
@justingrant
Copy link
Contributor

not stale. waiting for a maintainer to review #8227.

@stale stale bot removed the stale label Mar 24, 2020
@dbismut
Copy link
Contributor

dbismut commented Apr 17, 2020

For what it's worth — I don't know much about source maps or webpack so take my message with a grain of salt —, but I've made the following changes so that sourcemap works in CRA in my lerna monorepo.

As suggested, I've tweaked the config of CRA but using smart-source-map-loader which I believe fixes some issues of source-map-loader. Maybe @ AlexanderOMara can expend on this.

const enableWorkspaces = require('react-app-rewire-yarn-workspaces')

module.exports = function override(config, env) {
  config.module.rules.splice(1, 0, {
    enforce: 'pre',
    exclude: /@babel(?:\/|\\{1,2})runtime/,
    test: /\.(js|mjs|jsx|ts|tsx|css)$/,
    use: 'smart-source-map-loader',
  })
  return enableWorkspaces(config, env)
}

I've also added this line to the webpack config of other packages used by CRA, inspired by this webpack/webpack#3603 (comment)

{
  output: {
    // ...
    //fixes source maps
    devtoolModuleFilenameTemplate: '[absolute-resource-path]',
  },
  devtool: 'cheap-module-source-map',
  // ...
}

@stale
Copy link

stale bot commented May 17, 2020

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale
Copy link

stale bot commented Jun 17, 2020

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jun 17, 2020
@dominique-mueller
Copy link

Not stale.

@stale stale bot removed the stale label Jun 17, 2020
@stale
Copy link

stale bot commented Jul 18, 2020

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jul 18, 2020
@dominique-mueller
Copy link

Not stale.

@stale stale bot removed the stale label Jul 18, 2020
@stale
Copy link

stale bot commented Aug 22, 2020

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Aug 22, 2020
@JeremyGrieshop
Copy link
Author

Not stale!

@stale stale bot removed the stale label Aug 22, 2020
@stale
Copy link

stale bot commented Oct 4, 2020

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Oct 4, 2020
@Bessonov
Copy link

Bessonov commented Oct 4, 2020

The stale bot is one of the best ways to disrespect the community.

@stale stale bot removed the stale label Oct 4, 2020
@JeremyGrieshop
Copy link
Author

As of [email protected], the config-overrides.js will no longer work and will throw the "Unexpected CRA config. Exiting." The rules that get passed in now have a length of 2 now. I didn't know if the new rules would matter much and just took a chance and commented out the throw new Error('Unexpected CRA config. Exiting.');. Everything still works as expected, but I thought @justingrant may have a better suggestion here.

@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Dec 25, 2020
@Bessonov
Copy link

The stale bot is one of the best ways to disrespect the community.

@stale stale bot removed the stale label Dec 25, 2020
@JordanPawlett
Copy link

On going for over a year now. Has anyone got any interest in fixing support for source-map from imported modules?
My dev team and I use a private npm registry to split separation of concern into many packages, and i'm positive many many others do too!
I imagine this issue has led to many people moving away from CRA.

@vonkanehoffen
Copy link

So I also have this problem. I created a sample repo here https://github.com/vonkanehoffen/cra-monorepo-sourcemap-bug that demonstrates it if that helps any :-)

...I think this is the same issue anyway.

Very interested to see if #8227 makes it in! :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants