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 DX for shared TS codebase #4

Closed
wants to merge 10 commits into from
Closed

Improve DX for shared TS codebase #4

wants to merge 10 commits into from

Conversation

cayter
Copy link

@cayter cayter commented Apr 3, 2022

Thank you for helping out! ✨

We really appreciate your commitment to improving Architect

To maintain a high standard of quality in our releases, before merging every pull request we ask that you've completed the following:

  • Forked the repo and created your branch from master
  • Made sure tests pass (run npm it from the repo root)
  • Expanded test coverage related to your changes:
    • Added and/or updated unit tests (if appropriate)
    • Added and/or updated integration tests (if appropriate)
  • Updated relevant documentation:
  • Summarized your changes in changelog.md
  • Linked to any related issues, PRs, etc. below that may relate to, consume, or necessitate these changes

Please also be sure to completed the CLA (if you haven't already).

Learn more about contributing to Architect here.

Thanks again!

@CLAassistant
Copy link

CLAassistant commented Apr 3, 2022

CLA assistant check
All committers have signed the CLA.

@ryanblock
Copy link
Member

Thanks! Although this PR doesn’t address what we discussed in the issue, which is to recompile tsconfig paths. We should not assume src/shared is significant to TS users.

@cayter
Copy link
Author

cayter commented Apr 3, 2022

Thanks! Although this PR doesn’t address what we discussed in the issue, which is to recompile tsconfig paths. We should not assume src/shared is significant to TS users.

The code section is only recompiling typescript handlers if the file ends with .ts or .tsx which should work fine right? Is there any potential issues I'm missing out on that this change would cause? Thanks.

@ryanblock
Copy link
Member

ryanblock commented Apr 3, 2022

It will work for some, but this is more along the lines of the Architecty way, not the TypeScripty way. The approach we've tried to take for this plugin was to build it in the most TypeScripty way possible. In the example app we demonstrate using both src/views (not accounted for in this PR) as well as a custom TS shared dir: https://github.com/architect-examples/typescript-example/blob/main/tsconfig.json

If we are going to look at src/shared we are also going to need to look at src/views, but no mater what I think we're going to need to look at tsconfig paths, as that is currently the canonical method for declaring shared code in a TS project. We'd do that here: https://github.com/architect/plugin-typescript/blob/main/src/index.js#L39

@cayter
Copy link
Author

cayter commented Apr 4, 2022

@ryanblock I've updated the logic to use tsconfig.json compilerOptions.paths. Could you give another review? Thanks.

@cayter cayter changed the title Improve DX for shared TS codebase in src/shared Improve DX for shared TS codebase Apr 4, 2022
return
}
}

Copy link
Member

Choose a reason for hiding this comment

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

This looks good otherwise, although we still need tests!

Copy link
Author

Choose a reason for hiding this comment

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

@ryanblock I've updated the logic to:

  • take care of windows path
  • take care of non TS/TSX paths alias


let enPath = join(mock, 'defaults', 'src', 'shared', 'locales', 'en.ts')
let oldEn = readFileSync(enPath)
writeFileSync(enPath, 'export default "bar";\n')
Copy link
Author

Choose a reason for hiding this comment

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

@ryanblock Is there a way to start the sandbox with watcher? It seems like we can only start sandbox with watcher using CLI...

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants