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

Improve absolute import parsing for editors by moving baseUrl #7964

Closed
nitzano opened this issue Oct 7, 2017 · 13 comments
Closed

Improve absolute import parsing for editors by moving baseUrl #7964

nitzano opened this issue Oct 7, 2017 · 13 comments
Labels
area: @schematics/angular feature: insufficient votes Label to add when the not a sufficient number of votes or comments from unique authors feature Issue that requests a new feature needs: investigation Requires some digging to determine if action is needed
Milestone

Comments

@nitzano
Copy link

nitzano commented Oct 7, 2017

Bug Report or Feature Request (mark with an x)

- [] bug report -> please search issues before submitting
- [x] feature request

Versions.

@angular/cli: 1.4.4
node: 6.11.3
os: linux x64
@angular/animations: 4.4.4
@angular/common: 4.4.4
@angular/compiler: 4.4.4
@angular/core: 4.4.4
@angular/forms: 4.4.4
@angular/http: 4.4.4
@angular/platform-browser: 4.4.4
@angular/platform-browser-dynamic: 4.4.4
@angular/router: 4.4.4
@angular/cli: 1.4.4
@angular/compiler-cli: 4.4.4
@angular/language-service: 4.4.4
typescript: 2.3.4

Repro steps.

This improvements refers mainly to vscode/#24362 but i think it will be reproduced by other editors in the future as well. Currently the main (and only) typescript config file parsed by editors is tsconfig.json. Since the files ./src/tsconfig.app.json and ./src/tsconfig.spec.json both contains"baseUrl": "./" but there is no baseUrl property in ./tsconfig.json which they both extend there is no way for the common editor to know what the main baseUrl is and it will struggle with resolving and refactoring absolute imports (although compiling with ng build works fine).

The log given by the failure.

For example just "out of the box" app after ng new and changing one relative import to absolute one produces :

import_fail

Desired functionality.

if (as I wrote here) the baseUrl property will be moved from ./src/tsconfig.app.json and ./src/tsconfig.spec .json files to ./tsconfig.json and it will hold there "baseUrl": "./src/" it will solve the issue and will create better parsing for other editors in the future.

Mention any other details that might be useful.

  1. vscode/24362
  2. vscode/12463
@delasteve
Copy link
Contributor

This has been discussed at length multiple times. Currently, the team thinks it's best how it is and if you would like to use absolute paths, you will have to change it yourself.

(Some) Relevant issues/PRs:
#5875
#6861

@filipesilva
Copy link
Contributor

filipesilva commented Oct 8, 2017

I think @delasteve's answer covers the why of things right now pretty well.

It's important to note that in #5875, the problem was that having baseUrl set meant vscode defaulted to using absolute urls. That is unexpected and wouldn't work well with completion for e2e specs.

I think this is overall a small QoL adjustment that any user can do to their project. I don't think it should be the default because:

  • it messes up path completion for e2e specs
  • it defaults edits to using absolute paths (whereas currently they will default to using relative paths) and that would be an unexpected change
  • it requires messing with the default blueprints/schematics, both the tsconfig and the generated files to be consistent
  • wouldn't work well for multi-app setups (https://github.com/angular/angular-cli/wiki/stories-multiple-apps)

@nitzano
Copy link
Author

nitzano commented Oct 8, 2017

@filipesilva can you please demonstrate why it would mess up path completion for e2e specs or multi-app setups?

Also just to add more views: vscode project itself is using absolute imports over relative ones and using absolute imports is even preferred in other languages (e.g Python). Also just checked with latest vscode 1.17.0 that comes with import resolving and it didn't fallback to absolute imports automatically but let the user choose how to solve it (relative imports were even the first option after baseUrl was set).

All in all I can do this change for myself and adjust it in my project but it made more sense to maybe not to have baseUrl at all in ./src/tsconfig.app.json and ./src/tsconfig.spec.json over currently compiling with baseUrl in them but editing without the editor knowing about it as these files are ignored.

I agree that this is a small change that any user can do but without it absolute import paths won't work "out of the box" in a new app.

@filipesilva
Copy link
Contributor

Well if you change "baseUrl": "./src/" you're telling your editor that ./src/ is where your code is. But the e2e tests are not in ./src/, they are in ./e2e/.

And if you have multiple apps under ./src/, for instance ./src/app1 and ./src/app2/, then your baseUrl will be incorrect for all of them.

Your change works for your specific case because you have a single app and don't care much about the e2e folder, but that's not generalizable to all cases the CLI covers. So that would be a bad default to add.

@victornoel
Copy link

@filipesilva I see you reopened this issue, if it's not a mistake, I would say we could simply set baseUrl to src in the root of the project and rename tsconfig.e2e.json to tsconfig.json.

I think this should cover everything, no?

@vivainio
Copy link

Bumped into this myself just now. Fix was to add baseUrl: './src' to tsconfig.json.

It's a bit suboptimal that this is broken out of the box on vscode. Until vscode gets a feature to select tsconfig.app.json, it would be a good idea to do this in the default tsconfig.json.

@filipesilva
Copy link
Contributor

@victornoel I reopened it because I closed it by mistake. Your proposed solution still breaks for multi-app projects like I mention in my previous comment.

@victornoel
Copy link

ok, noted, thx for the update :)

@vivainio
Copy link

vivainio commented Dec 7, 2017

@filipesilva It would seem more sensible to have correct behavior for the 95% use case (users starting new apps with angular-cli). Once you go multi-app, you can find out how to change the config yourself.

@ngbot ngbot bot added this to the needsTriage milestone Oct 1, 2019
@filipesilva filipesilva added the feature Issue that requests a new feature label Oct 1, 2019
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Oct 1, 2019
@alan-agius4 alan-agius4 added the needs: investigation Requires some digging to determine if action is needed label Oct 22, 2020
@angular-robot
Copy link
Contributor

angular-robot bot commented Feb 1, 2022

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

@angular-robot angular-robot bot added the feature: votes required Feature request which is currently still in the voting phase label Feb 1, 2022
@ngbot ngbot bot modified the milestones: Backlog, needsTriage Feb 1, 2022
@angular-robot
Copy link
Contributor

angular-robot bot commented Feb 21, 2022

Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.

We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.

You can find more details about the feature request process in our documentation.

@angular-robot angular-robot bot removed the feature: votes required Feature request which is currently still in the voting phase label Feb 21, 2022
@angular-robot angular-robot bot added the feature: insufficient votes Label to add when the not a sufficient number of votes or comments from unique authors label Feb 21, 2022
@alan-agius4
Copy link
Collaborator

Closing as I believe this is solved by #25934. If the issue still persists please open a new issue.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: @schematics/angular feature: insufficient votes Label to add when the not a sufficient number of votes or comments from unique authors feature Issue that requests a new feature needs: investigation Requires some digging to determine if action is needed
Projects
None yet
Development

No branches or pull requests

7 participants