Skip to content
This repository was archived by the owner on Sep 8, 2021. It is now read-only.

Don't export const enums? #56

Closed
oliverlloyd opened this issue Jan 14, 2021 · 2 comments · Fixed by #59
Closed

Don't export const enums? #56

oliverlloyd opened this issue Jan 14, 2021 · 2 comments · Fixed by #59

Comments

@oliverlloyd
Copy link
Contributor

Shall we replace our use of exported const enums with simple enums?

I've been spending the past week or so trying to get the Design, Display and Theme const enums supported in DCR. I did the work to first bring them into DCR's dependencies such as atoms-rendering and discussion-rendering and then went through DCR's code, refactoring away from string to the const enum pattern.

This work was difficult and took a lot if time but I was able to get atoms and discussion working. However, when trying to bring v1.1.0 of this lib into DCR I am facing a serious problem. This issue well describes where we are.

TypeStrong/ts-loader#331 (comment)

and

microsoft/TypeScript#16671 (comment)

The conclusion here is you turn off transpileOnly to workaround the issue but for DCR that has 2 costs:

  1. It throws up additional errors. These may be solvable but I've not got to the bottom of them yet and
  2. our build times increase substantially

I've been spending a large part of today working through this but I am starting to think we may have reached the point where const enums are not worth the amount of time they are costing. To be clear, I want them to work and I am not looking forward to having to again refactor the code to move to a new pattern, but my worry is even if we do eventually work out a solution, there's a very real chance the next project which tries to import these types from this repo will have similar problems.

@JamieB-gu
Copy link
Contributor

JamieB-gu commented Jan 14, 2021

Having discussed this with @oliverlloyd earlier today I think doing this is reasonable, at least for now. I'd still like to get to the bottom of why DCR builds are so resource-intensive compared to AR builds, and maybe if we figure that out we can go back to const enums. I believe the ideal solution is to get DCR working without transpileOnly, because then it can be compiled with the full TS feature-set. However, this is blocking other work getting done on DCR and Oliver has already expended a lot of effort looking into the DCR issues.

One thing to note is that the size of AR bundles (and other projects using types) will increase as a result of this - I can investigate by how much if we create a branch with these changes.

I am not looking forward to having to again refactor the code to move to a new pattern

I think the lookup code for enums and const enums is the same, so there may not be much to refactor?

const enum Foo {
  Bar
}

const baz: Foo = Foo.Bar;
enum Foo {
  Bar
}

const baz: Foo = Foo.Bar;

@oliverlloyd
Copy link
Contributor Author

I think the lookup code for enums and const enums is the same, so there may not be much to refactor?

🤞 🤞 🤞 🤞 🤞 🤞 🤞 🤞 🤞 🤞 🤞 🤞 🤞

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

Successfully merging a pull request may close this issue.

2 participants