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 #854

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from
Open

Conversation

AntoineBalaine
Copy link

Absolutely don't merge this YET!

Hello Mr. Rosen,

Sorry to have been so MIA these past few months. I finally found some time to work on the switch to TS this week.

Here's the migration to typescript we had discussed. It's still in draft state, but I wanted to submit it to you asap, so you'd be able to object to anything that doesn't fly.

  • I used ts-migrate for the migration.
  • updated webpack and babel to compile (mostly working, except for the abcjs-plugin, which has some issues)
  • switched requires and modules.exports to imports and exports
  • added prettier and eslint to the project - eslint is disabled for now, as the current setup has conflicts between the two. This was a necessary addition to do the migration
  • in package.json: project's "type" has been set to "module" in order to use imports and exports. I don't know if this potentially breaks anything for users of the library? The output still uses es5, though.
  • tests haven't been updated yet: links to js files are broken and I believe that <script> imports in the html files of the tests might require an "type=module" tag

That's all I have for now, I hope you'll find this PR useful!

Cheers

Anthony Balaine added 24 commits November 18, 2022 15:37
In the process of preparing the codemod that will operate the switch
to typescript, I'm having to add eslint and prettier: both of these
are going to be used by the codemod tool to format the code and detect
types and errors at compile time.
I do realize that Paul Rosen probably already has an eslintrc somewhere,
I'm not re-adding it here. Would it be useful to have it included in the
repo in the future?

add prettier ignore config
switch hasOwnProperty to
prototype.hasOwnProperty.call
convert errors to warnings
I'm switching webpack to compile TS files here.
With the update, webpack compiles the library normally, but has some
issues with the plugin version: it can't resolve the `TuneBook` import
and for some reason, the logic of that component eludes me enough that I
can't fix it right away.
Import the property directly from package.json was causing issues, so I had
to find a way to use «require» instead of «import» for this one piece of
data.
@AntoineBalaine AntoineBalaine marked this pull request as ready for review November 20, 2022 19:04
@paulrosen
Copy link
Owner

Hi Antoine, the first thing I notice is that you've requested a merge to main. Please merge to dev instead. I push to main only when I'm creating a release.

I'll look at the actual code this week.

@AntoineBalaine AntoineBalaine changed the base branch from main to dev November 28, 2022 01:50
@AntoineBalaine
Copy link
Author

Sounds good, I changed the destination!

@paulrosen
Copy link
Owner

First thing I noticed is that the copyright notice is not on top of the abcjs-basic.min.js file.

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.

None yet

2 participants