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

Convert imports to non-relative, format all code using TS Server #2007

Closed
wants to merge 8 commits into from

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Apr 10, 2019

We have ./, ../ and ../../ scattered all over out imports which makes it hard to tell at a glace which imports are coming from where. This PR converts everything over to non-relative via the baseUrl TS compiler option. Some other things of note:

  • The baseUrl for all projects is the repo root, this means that all imports consistently start with 'src/...' and are sorted nicely (plus we need to do this for files in src/ to be non-relative)
  • A VS Code settings file is added to give hints to the editor to use single quotes and to import non-relative, I think this is the only way to get it to work within VS Code.
  • Format on save for VS Code is also turned on so spacing inconsistencies are a thing of the past, this breaks how we were spacing out code in some places but imo it's best to embrace how the TS formatter does things (and there's no setting for that).
  • TS auto imports will still import from lib for project references, there's another issue still open for this.
  • I've organized imports using TS Server for all files, it seemed to act a little weird for composite projects though as it will open the generated .d.ts file and make the changes there. I organized core/common manually. This will be a bit of a pain for PRs but it's a one time thing and the conflicts are fairly easy to fix since the compiler tells you what's missing.

Fixes #1314

This is necessary because of all the files that currently live in src/.
Once they're moved into folders in src we could remove this but it's
probably a good idea that all xterm.js imports start with 'src/...'.
@Tyriar Tyriar added this to the 3.13.0 milestone Apr 10, 2019
@Tyriar Tyriar self-assigned this Apr 10, 2019
@Tyriar
Copy link
Member Author

Tyriar commented Apr 10, 2019

Need to fix up the browserify build

@Tyriar
Copy link
Member Author

Tyriar commented Apr 10, 2019

Looks like this is a lot more complicated to get right since the src/... part is actually emitted into lib, we would need to lay out the code like this:

  • src
    • xterm
      • common
      • other files/folders

With imports like this 'xterm/common...'. Then it would also run into issues when the lib version is used because of this TS issue microsoft/TypeScript#10866 (closed as designed). It could be done if we packaged up the lib version instead of having all the files split up.

@Tyriar Tyriar added the work-in-progress Do not merge label Apr 10, 2019
@Tyriar Tyriar removed this from the 3.13.0 milestone Apr 10, 2019
@Tyriar
Copy link
Member Author

Tyriar commented Apr 11, 2019

Bit of a shame but I'm going to close this off, I don't think we can do this and also keep lib how we want to. Will add single quote vscode setting in a separate PR as that will save a bunch of changing " to ' when using auto imports/implement interface/etc.

@Tyriar Tyriar closed this Apr 11, 2019
@Tyriar Tyriar deleted the 1314_non_relative branch April 11, 2019 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress Do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use tsconfig baseUrl and absolute imports
1 participant