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

Allow JSON files to be used as modules in ts_library #2264

Closed
wants to merge 2 commits into from

Conversation

vpanta
Copy link
Contributor

@vpanta vpanta commented Nov 2, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

ts_library does not allow you to have a JSON file as a source, nor does it let you import from them.

Issue Number: #1109

What is the new behavior?

Adds a allow_json_srcs attr to ts_library for those who would like to use JSON as source files (assuming the user's tsconfig and chosen module output types allow it).

If allow_json_srcs is not supplied, ts_library should effectively have no change.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I wasn't sure what files to update for documentation, and I understand if this isn't something y'all want to take on.

@alexeagle
Copy link
Collaborator

You checked the box in the PR description for "tests added" but I don't see them here. Could you add a .json input under whichever existing packages/typescript/tests/* seems most appropriate and some assertion of what you expect to happen, like that the program type-checks or that the json file got copied to the output tree?

@vpanta
Copy link
Contributor Author

vpanta commented Nov 3, 2020

You checked the box in the PR description for "tests added" but I don't see them here. Could you add a .json input under whichever existing packages/typescript/tests/* seems most appropriate and some assertion of what you expect to happen, like that the program type-checks or that the json file got copied to the output tree?

Ugh, sorry, I completely missed that I didn't git add it.

I've added some tests, but I'd appreciate some pointers for testing some of the mechanical pieces. For instance, I'd love to verify that the build fails if I give a json file but don't set allow_json_srcs. And I'm not sure the best way to verify that the file ends up copied to the output tree.

@vpanta
Copy link
Contributor Author

vpanta commented Nov 5, 2020

Fudge, this might not be enough. I might also have to change the tsc_wrapped to not emit JSON.

Right now, if I build the ts_library as-is first, and then later build it with something which ingests say, es5_sources, it works fine.

However, if I build es5_sources from a clean branch, then I get

error TS5033: Could not write file '<path-to-output-json>': EACCES: permission denied, open '<path-to-output-json>'.

It's weird to me I don't get that every time, but it's coming from tsc attempting to output the JSON file but not actually having access in the sandbox to do so.

If I change rules_typescript to not emit JSON though, it works.

And to note: I cannot allow compile_action/devmode_compile_action to have the JSON in their outputs (even though it technically is) because then I get a Bazel error about 2 actions generating the same output target.

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

yeah your error does indicate a problem. If the file already exists in the output tree we aren't allowed to overwrite it unless we declare as an output.

The contract from tsc is that .json inputs are written to the output tree, so the correct thing to do under Bazel is to model it the same way. However as you point out we currently have these two different hard-coded output flavors in ts_library which is a big project to fix. (and maybe not worth it if ts_project catches up)

My guess is the flavor that outputs .d.ts files should also output .json files - I think that's the es5_sources output (devmode_compile_action)

I suppose hacking tsc_wrapped as a workaround is acceptable. It's just another admission that ts_library diverged from tsc in a way we can't fix with the resources we have to work on in.

})

# Unless this is set true, manually prevent JSON in the srcs attribute.
if not kwargs.get("allow_json_srcs", False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add some explanation why we need this, rather than always allow .json in srcs now?

@vpanta
Copy link
Contributor Author

vpanta commented Dec 18, 2020

I'm going to close this. Given what we last discussed, there's a bit more thought that needs put into it for what's best with ts_library's outputs here, and as 3.0.0 is around the corner, that'd be the right place to address it.

For now we're using a local fork, and will either merge forward to 3.* (with a future fix here) or will find a way to move over to ts_project if possible.

@vpanta vpanta closed this Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants