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

Typescript migration #600

Closed
wants to merge 2 commits into from
Closed

Conversation

zhouzi
Copy link
Contributor

@zhouzi zhouzi commented May 17, 2022

As suggested in #576, it would be great to have emoji-mart's types in its repository. @Scalahansolo started to work on it and had issues with parcel to generate the types (reported in parcel-bundler/parcel#8014). I am also interested in the types so to help move forward I went ahead and investigated the issue.

I am trying to keep the configuration as minimal as possible and with 2c2c515 I am able to build the library and generate the types. There is not much to generate for now but at least we got past the issue with parcel.

To be transparent, I don't know if I will have the time to migrate the whole codebase. We will probably create a declaration file on our end to move forward. I will post an update if it becomes clear that I won't be able to work on it on my spare time in the next few days, in case someone else wants to take over.

"parcel": "2.5.0",
"postcss": "8.4.13",
"preact": "10.6.4",
"typescript": "4.6.4"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As suggested in #576 (comment) we need to declare the dependencies explicitly due to the --no-autoinstall flag in the build script.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear: even with autoinstall, dependencies would still be added to package.json, but automatically by parcel. IIRC it’s not quite compatible with workspaces and when starting from scratch you get overwhelmed with a ton of dependencies you’re not sure you even need, so I prefer to handle them manually.

I don’t think postcss requires to be explicitly added, was it? I believe it’s already included (see yarn.lock) through parcel sub-dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dependencies I added so far were required, otherwise it was throwing a "{module} not found" error. I don't know why it is throwing for postcss though. I will try to remove it and/or figure out why it is required.

Copy link
Member

Choose a reason for hiding this comment

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

No worries. I’m still experimenting / figuring out the whole workspaces and the best way to configure everything. Are you running scripts from the root or from packages/emoji-mart? If the latter, that might be why (and also maybe why you got around the typescript issue from #576). With workspaces, you need to handle dependencies from the root. Dependencies can still be added to a specific workspace (like it currently is), but there’s only one node_modules at the root. Workspaces install all workspaces dependencies at the root.

We could even move all devDependencies to the root package.json if that helps with issues.

@Scalahansolo
Copy link
Contributor

Just wanted to check here quick and see if this is something we collectively want to tackle. The actual TS migration might take me a couple of hours to burn through and I think it would be an awesome addition.

@zhouzi
Copy link
Contributor Author

zhouzi commented Jul 20, 2022

Sorry, I wasn't able to work on this and we ended up using the previous version. Feel free to give it a go 💪

jwhitham-ds added a commit to jwhitham-ds/emoji-mart that referenced this pull request Aug 10, 2022
This helps with issues I ran in to surrounding @parcel/transformer-typescript-types
and is mentioned here:
missive#600
jwhitham-ds added a commit to jwhitham-ds/emoji-mart that referenced this pull request Aug 12, 2022
This helps with issues I ran in to surrounding @parcel/transformer-typescript-types
and is mentioned here:
missive#600
@zhouzi
Copy link
Contributor Author

zhouzi commented Aug 12, 2022

@jwhitham-ds in now working on this through #671 🙌

@zhouzi zhouzi closed this Aug 12, 2022
@zhouzi zhouzi deleted the typescript-migration branch August 12, 2022 07:41
jwhitham-ds added a commit to jwhitham-ds/emoji-mart that referenced this pull request Aug 26, 2022
This helps with issues I ran in to surrounding @parcel/transformer-typescript-types
and is mentioned here:
missive#600
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants