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

Enable click to go to error in console part 2! #6502

Merged
merged 4 commits into from Mar 10, 2019

Conversation

@johnnyreilly
Copy link
Contributor

commented Feb 23, 2019

This is a continuation of #5624 (which died tragically in a merging accident).

Please see context there.

@johnnyreilly

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

@johnnyreilly

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

This still has some problems which I'm working on fixing now.

@johnnyreilly

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

All good now 😄

I enclose some example screenshots illustrating the usage experience both in VS Code:

screenshot 2019-02-24 08 07 47

and in the dev overlay:

screenshot 2019-02-24 08 10 06

@ianschmitz ianschmitz added this to the 2.1.x milestone Feb 24, 2019

@ianschmitz

This comment has been minimized.

Copy link
Collaborator

commented Feb 24, 2019

In your dev overlay screenshot I notice the drive in the path is missing a slash. Was that existing behavior for you?

@ianschmitz ianschmitz self-assigned this Feb 24, 2019

@johnnyreilly

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

I will check for you and report back.

It's not existing behaviour. It's a side effect of making the change of tweaking it so the path prefix does not emit in the console (see here and here )

Now fixed 😄

@@ -332,7 +332,8 @@ Creates a Webpack compiler instance for WebpackDevServer with built-in helpful m
The `args` object accepts a number of properties:

- **appName** `string`: The name that will be printed to the terminal.
- **config** `Object`: The webpack configuration options to be provided to the webpack constructor.
- **appPath** `string`: The path to the application. Used to make error / warning paths pithier.

This comment has been minimized.

Copy link
@ianschmitz

ianschmitz Feb 24, 2019

Collaborator

Typo

This comment has been minimized.

Copy link
@johnnyreilly

johnnyreilly Feb 25, 2019

Author Contributor

what's the typo? Looks good to me? I can confirm that "pithier" is a word 😄 https://en.wiktionary.org/wiki/pithier

@@ -103,6 +103,7 @@ function printInstructions(appName, urls, useYarn) {

function createCompiler({
appName,
appPath,

This comment has been minimized.

Copy link
@ianschmitz

ianschmitz Feb 24, 2019

Collaborator

Maybe we should give this a default value of ''?

This comment has been minimized.

Copy link
@johnnyreilly

johnnyreilly Feb 25, 2019

Author Contributor

if we give it a default value of '' then it somewhat becomes purposeless; appPath is passed around so we can strip the prefix path from lots of places. eg C:/source/my-app/src/something.tsx -> src/something.tsx

I'm beginning to have mixed feelings about the path prefix stripping actually. Whilst it initially seemed like a good idea and reduced "noise" that rather enforces a certain way of working with your codebase. Let me explain:

Consider the following (not unusual) project structure:

package.json
src/
    client/
        src/
            package.json
            // ...
    server/
        src
            package.json
            // ...

With this structure it's quite common to want to open up in the root of that structure and work from there, hacking on both client and server at the same time. You may fire up the VS Code and, run a yarn start:client script which behind the scenes does this cd src/client && yarn start

Without the stripping path prefixes approach this works fine. The paths that appear in the console are clickable as they are complete paths. However with path stripping in effect VS Code doesn't know where to go when you're working in the root of the project. Where is this src/ you speak of? It's confused and not unreasonably.

We lose click to go to error in console entirely in this scenario due to path prefix stripping. You can work around it by opening up only in the root of the client folder and working there. Then it'll be fine. But this feels like we're rather forcing a way of working upon users.

What do you think? My feeling is that users would probably be better served if we dropped the path prefix stripping.

This comment has been minimized.

Copy link
@ianschmitz

ianschmitz Mar 9, 2019

Collaborator

Thanks for the detailed explanation.

I agree. All of our path stripping solutions so far have had caveats and been somewhat complicated. Maybe the best first implementation is to use full paths. If we can come up with a robust solution in the future, then it will be an improvement.

This comment has been minimized.

Copy link
@johnnyreilly

johnnyreilly Mar 10, 2019

Author Contributor

Solid - I'll make it so 👍

@@ -227,14 +230,14 @@ function createCompiler({
messages.errors.length = 1;
}
console.log(chalk.red('Failed to compile.\n'));
console.log(messages.errors.join('\n\n'));
console.log(messages.errors.join('\n\n').replace('//', ''));

This comment has been minimized.

Copy link
@ianschmitz

ianschmitz Feb 24, 2019

Collaborator

What is this and the following replace() for? The regex doesn't look right but I am low on coffee.

This comment has been minimized.

Copy link
@johnnyreilly

johnnyreilly Feb 25, 2019

Author Contributor

This is also related to path prefix stripping - both the console and the Error Overlay are driven from the same information. This removes a double // from the console that would otherwise be printed. Alas the // is necessary to make the Error Overlay display the colon as expected, eg C:/

See my other comment but I'm now of the opinion that path prefix stripping might not be a good idea. I'd be happy to back that out.

@johnnyreilly

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

Reverted the path amends - error / warnings in the console are clickable always 😄 and we have a full path as we do currently in create-react-app:

image

image

@ianschmitz ianschmitz modified the milestones: 2.1.x, 2.1.9 Mar 10, 2019

@ianschmitz ianschmitz merged commit bac0287 into facebook:master Mar 10, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/create-react-app/deploy-preview Docs deploy preview succeeded
Details
@ianschmitz

This comment has been minimized.

Copy link
Collaborator

commented Mar 10, 2019

Thanks @johnnyreilly!

@iansu iansu modified the milestones: 2.1.9, 3.0 Mar 10, 2019

@iansu iansu added this to Done in v3 Mar 10, 2019

JoviDeCroock added a commit to JoviDeCroock/create-react-app that referenced this pull request Mar 15, 2019

Merge branch 'masterd' into feat/modern-build
* masterd: (24 commits)
  Add TypeScript linting support (facebook#6513)
  Support React Hooks (facebook#5602) (facebook#5997)
  Support browserslist in @babel/preset-env (facebook#6608)
  Add empty mock for http2 (facebook#5686)
  Add note about npx caching (facebook#6374)
  change named import into default import (facebook#6625)
  Stage files for commit after ejecting (facebook#5960)
  Upgrade dependencies (facebook#6614)
  Make compiler variable const instead of let (facebook#6621)
  Type check JSON files (facebook#6615)
  Change class components to functional components in templates (facebook#6451)
  Convert JSON.stringify \n to os.EOL when writing tsconfig.json (facebook#6610)
  Update html-webpack-plugin (facebook#6361)
  Enable click to go to error in console for TypeScript (facebook#6502)
  Update webpack-dev-server to 3.2.1 (facebook#6483)
  [docs] revert removal of newlines from html (facebook#6386)
  Publish
  Prepare 2.1.8 release
  Reapply "Speed up TypeScript v2 (facebook#6406)" (facebook#6586)
  Publish
  ...

# Conflicts:
#	packages/babel-preset-react-app/create.js
#	packages/react-scripts/scripts/build.js

@lock lock bot locked and limited conversation to collaborators Mar 15, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.