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

Add node trace source map handling in dev mode #2816

Merged
merged 2 commits into from
Aug 30, 2017

Conversation

kpdecker
Copy link
Contributor

Fixes #2285

@erikras
Copy link

erikras commented Aug 22, 2017

This will literally save hours of dev time, as it is currently so non-obvious what line is causing an error. 👍 👍

@timneutkens
Copy link
Member

This looks really good, let me give it a few spins before merging 👌

@ptomasroos
Copy link
Contributor

This is brilliant @kpdecker !

server/index.js Outdated
@@ -29,6 +29,14 @@ const blockedPages = {

export default class Server {
constructor ({ dir = '.', dev = false, staticMarkup = false, quiet = false, conf = null } = {}) {
// When in dev mode, remap the inline source maps that we genenrate within the webpack portion
Copy link
Member

Choose a reason for hiding this comment

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

small typo: genenrate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

Choose a reason for hiding this comment

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

❤️

@arunoda
Copy link
Contributor

arunoda commented Aug 28, 2017

This looks like something really great.
I'll give this a run and merge this.

@timneutkens
Copy link
Member

Tried it last night, works 👌

@arunoda
Copy link
Contributor

arunoda commented Aug 30, 2017

This is fantastic.
Thanks for the hard work @kpdecker

@arunoda arunoda merged commit d600957 into vercel:master Aug 30, 2017
@arunoda
Copy link
Contributor

arunoda commented Aug 30, 2017

I just published 3.2.0 which has this PR.

@dnakov
Copy link

dnakov commented Aug 30, 2017

This broke browser source maps.
To reproduce, run an example (e.g. with-absolute-imports) with next@3.2.0.
Source maps look like this:

null


// WEBPACK FOOTER //
// ./components/header.js

@kpdecker
Copy link
Contributor Author

I can't really make sense of that output. What command is it from? What does your nextconfig look like?

@kpdecker
Copy link
Contributor Author

And what browser?

@dnakov
Copy link

dnakov commented Aug 30, 2017

Chrome Version 60.0.3112.101 (Official Build) (64-bit) / MacOS 10.12.6 / Node 8.1.2

As I said above, just running the with-absolute-imports example in the repo with next@3.2.0 and looking at the source maps in chrome devtools.
The output is what chrome shows as the source-mapped source for components/header.js
screen shot 2017-08-30 at 3 46 34 pm

@timneutkens
Copy link
Member

I'm thinking this is most likely related to the absolute imports example 🤔 cc @sergiodxa

@dnakov
Copy link

dnakov commented Aug 30, 2017

I just tried a random example -- with-redux, same result.
Downgraded to next@3.0.6 and the source maps came back.

@KClough
Copy link

KClough commented Sep 3, 2017

I'm also experiencing the same issue as @dnakov , I reverted back to next@3.1.0 and no longer see an issue with sourcemaps while debugging.

@kpdecker
Copy link
Contributor Author

kpdecker commented Sep 3, 2017

Fixed in #2900.

@kpdecker kpdecker deleted the node-source-maps branch September 21, 2017 17:57
@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants