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 F12 navigation in vscode #72

Closed
wants to merge 3 commits into from
Closed

Enable F12 navigation in vscode #72

wants to merge 3 commits into from

Conversation

elevatebart
Copy link
Contributor

@elevatebart elevatebart commented Aug 4, 2018

As specified in this doc, in order for vscode to understand webpack aliases, it needs a little help.
In order to keep the code as DRY (don't repeat yourself) as possible,
I moved the aliases definition in the static jsconfig.json file.

nvopt

As specified in this doc
https://code.visualstudio.com/docs/languages/jsconfig#_using-webpack-aliases
In order for vscode to understand webpack aliases,
it needs a little help.
In order to keep the DRY (don't repeat yourself),
I "moved" the aliases definition in the static json file.
@saintplay
Copy link

This is a huge deal for VSCode, because auto completion, refactoring, Intellisense and all vscode incoming features wouldn't work without this.

I would add these lines to the jsconfig.json too

    "include": ["src/**/*", "tests/**/*"],

This jsconfig.json should be included in the boilerplate, it's really a game changer 👌 👌 👌

@elevatebart
Copy link
Contributor Author

I agree, I just don't know if @chrisvfritz wants to keep the boilerplate editor agnostic.

@elevatebart
Copy link
Contributor Author

And I just found this issue,
vuejs/vetur#547

vetur is not a friend of jsconfig
jsconfig does not seem to resolve .vue files very smoothly.

I will try a bit harder.

By default, vscode does not navigate to `.vue` files.
Once again, let us help it a little and add navigation 
using a vscode plugin: 
https://marketplace.visualstudio.com/items?itemName=dariofuzinato.vue-peek
@frandiox
Copy link
Contributor

I've been using jsconfig.json with this template for a while and I also agree it improves DX a lot 👍 . I removed aliases.config.js file since it doesn't actually contain aliases anymore, it just changes the format for webpack and jest (example here).

@chrisvfritz
Copy link
Collaborator

chrisvfritz commented Aug 26, 2018

I agree this is a great improvement! I just tried to implement this in a way that still allows programmatic generation of aliases with aliases.config.js. 🙂

@elevatebart @frandiox @saintplay What do you think of this commit?

@chrisvfritz
Copy link
Collaborator

Closing for now, since this has now been implemented, but this thread is still open to discussion. 🙂

@elevatebart
Copy link
Contributor Author

elevatebart commented Aug 29, 2018

Hey @chrisvfritz, you found a great way to keep backward compatibility. I love it.
My way was prventing anybody that had changed the aliases from using the new commit without changing their code.

I do not enjoy having both generated and versionned code in jsconfig.json.
My guts tell me that mixing generated and versionned code can lead to confusing commits.
The only alternative I see would be to have a versionned jsconfig.template.json containing the static part of jsconfig.json.
This way we could generate and gitignore the actual jsconfig.json.

It is way to complicated for something that virtually never changes.
Reflection to be continued

Thank you.

@chrisvfritz
Copy link
Collaborator

chrisvfritz commented Aug 29, 2018

@elevatebart You give me too much credit. 😅 What I implemented would actually overwrite anyone's existing jsconfig, based on what's in aliases.config.js, so it's purely generated right now. In the meantime, I just added it to .gitignore as you're right - that's where it belongs as a dev/build artifact.

So right now, any static config for jsconfig.json would have to be added to aliases.config.js, which isn't ideal because it may not be the first place someone would look for it. 🤔 I also worry that a jsconfig.template.json could cause similar confusions, unless maybe it's JSONC so I could add comments. I wonder if Node is able to import JSONC? If so, that might be the ideal solution.

What do you think?

@elevatebart
Copy link
Contributor Author

My ideal solution is not ideal.
It will force someone that has changed aliases.config.js to redo the same changes in jsconfig.json.
It resembles both what @frandiox and I have done. Let me try to illustrate it in this branch.

The core idea goes as follows: Since jsconfig.json cannot be processed, it cannot take values coming from other files. We should use it as the single source of truth. This way aliases.config.js, vue.config.js and jest.config.js aliases will follow jsconfig.json.

@elevatebart
Copy link
Contributor Author

@chrisvfritz, I updated the branch with the last of my reflexions. I as well updated the documentation. I can't wait to read your feedback

@chrisvfritz
Copy link
Collaborator

chrisvfritz commented Aug 31, 2018

@elevatebart I definitely do want to keep the aliases.config.js, because I've found it very helpful for visibility - people see it and immediately know exactly where to configure aliases, whereas jsconfig.js is a little more opaque.

I updated what I had to use a template as you suggested. What do you think now?

@elevatebart
Copy link
Contributor Author

elevatebart commented Aug 31, 2018

@chrisvfritz I like it a lot. I still have a question:

  • How to make it obvious that one has to run the yarn dev or yarn build command to be able to navigate?

I see that it's not generated in _start.js, that is a first solution.

I have another solution to suggest that would keep the aliases.config in the mix:
I know one can "extend" a jsconfig.json file.
Would it be ok to have a aliases.config.json instead of aliases.config.js?

I have illustrated this last solution in this branch.

What do you think?

@chrisvfritz
Copy link
Collaborator

I see that it's not generated in _start.js, that is a first solution.

Agreed! I'll add that. 🙂

I have illustrated this last solution in this branch.

That's a nice compromise, but it would still be difficult to lose the JS file. I've run into rare cases where some aliases should point to different paths depending on the environment. We could possibly do this work in the aliases-adapter.js file you set up, but then aliases.config.json would no longer be the real source of truth for aliases, which could cause confusion.

Ultimately, since I and others are using this for a large variety of projects, I think I have to value adaptable patterns over perhaps more elegant ones. Does that make sense?

@elevatebart
Copy link
Contributor Author

Makes perfect sense. Especially the consistency part. Glad I could be of help a little ;-)

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

Successfully merging this pull request may close these issues.

4 participants