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

Migrate to esbuild #70

Merged
merged 18 commits into from
Mar 15, 2021
Merged

Migrate to esbuild #70

merged 18 commits into from
Mar 15, 2021

Conversation

splashsky
Copy link

This PR addresses #69 (nice), implementing the migration to the esbuild bundler. A lot of the Webpack bloat is removed and the list of required dependencies is now much shorter.

This PR also accidentally includes the removal of the now-useless docs/ directory, which was meant to take effect when the new docs were pushed through.

See issue #69 in regards to this PR's functionality - a working example of the esbuild build can be found there.

Copy link
Owner

@Annoraaq Annoraaq left a comment

Choose a reason for hiding this comment

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

Impressive how much config code we can save.

I found 2 problems:

  1. TypeScript dev dependency needs to stay, otherwise the tests can't run (npm test)
  2. Because the tsconfig.json is missing there are 2 issues:
    2.1 the tests don't run because of the '--downlevelIteration' flag
    2.2 the type declaration files are not being generated (.d.ts files).

I think we can't get rid of the tsconfig.json because it is necessary for some ts settings.

Copy link
Owner

@Annoraaq Annoraaq left a comment

Choose a reason for hiding this comment

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

Impressive how much config code we can save.

I found 2 problems:

  1. TypeScript dev dependency needs to stay, otherwise the tests can't run (npm test)
  2. Because the tsconfig.json is missing there are 2 issues:
    2.1 the tests don't run because of the '--downlevelIteration' flag
    2.2 the type declaration files are not being generated (.d.ts files).

I think we can't get rid of the tsconfig.json because it is necessary for some ts settings.

@splashsky
Copy link
Author

I re-added the typescript dev dependency and brought back tsconfig, ran the tests successfully on my end. Check it out and see if we're good to go 😄

@splashsky splashsky requested a review from Annoraaq March 12, 2021 19:05
@splashsky splashsky self-assigned this Mar 12, 2021
@splashsky splashsky added dependencies Pull requests that update a dependency file enhancement New feature or request labels Mar 12, 2021
@splashsky splashsky linked an issue Mar 12, 2021 that may be closed by this pull request
@Annoraaq
Copy link
Owner

@splashsky The tests run fine now. But if I run npm run build I still don't get the declaration files despite the setting in the tsconfig.

I found this issue here:

evanw/esbuild#95

They suggest to create the declaration files in a separate step with tsc --emitDeclarationOnly.

If I chain the commands in the package.json like this:

"scripts": {
        "build": "tsc --emitDeclarationOnly && esbuild src/main.ts --bundle --minify --global-name=GridMovementPlugin --outfile=dist/GridMovementPlugin.min.js"
    },

It creates the declaration files. However, I still need to manually copy the GridMovementPlugin.d.ts file to the dist folder.

@Annoraaq
Copy link
Owner

we could add another dependency to do that (i.e. https://www.npmjs.com/package/copyfiles)

@splashsky
Copy link
Author

splashsky commented Mar 12, 2021

Actually, I was able to add a small modification to the build script that generates type declarations and dumps them into dist/, although it generates .d.ts files for everything in src/. Is this expected behavior? Should I be dumping the declarations into src/ instead?

It seems that bundling type declarations still has iffy support across the board. ☹️

Copy link
Owner

@Annoraaq Annoraaq left a comment

Choose a reason for hiding this comment

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

Please turn back the indentation. But the rest works fine. Actually I found out that it is even better to have all the .d.ts files around because they are including each other.

So type support should be better now. 🙂

src/GridMovementPlugin.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@splashsky splashsky requested a review from Annoraaq March 14, 2021 16:43
@splashsky
Copy link
Author

The recent PRs kind of went beyond the scope of this little project, but I've got prettier and eslint set up here as well. Tested them in the dev script and they're working fine - builds also are good!

Sorry about the spaces - my IDE is fully set up to do everything in 4 spaces automatically so I didn't even notice 🤣

@Annoraaq
Copy link
Owner

Annoraaq commented Mar 15, 2021

@splashsky with "recent PRs" you mean adding eslint and prettier?

I never heard that using prettier and eslint depends on the size of the project before. Despite being useful for a single person already, their true advantage shows when having more than one person working at the code.

tsconfig.json Show resolved Hide resolved
@splashsky
Copy link
Author

@splashsky with "recent PRs" you mean adding eslint and prettier?

I never heard that using prettier and eslint depends on the size of the project before. Despite being useful for a single person already, their true advantage shows when having more than one person working at the code.

I apologize, I meant little project as in this PR specifically - but I did get that last note resolved so maybe we're good to go? 😄

@Annoraaq
Copy link
Owner

@splashsky with "recent PRs" you mean adding eslint and prettier?
I never heard that using prettier and eslint depends on the size of the project before. Despite being useful for a single person already, their true advantage shows when having more than one person working at the code.

I apologize, I meant little project as in this PR specifically - but I did get that last note resolved so maybe we're good to go? 😄

No, I apologize 🙂 I now get what you meant. Now everything works fine. We are good to go. Excellent work and thank you for your contribution. esbuild is blazingly fast.

@Annoraaq
Copy link
Owner

I can't merge it due to conflicts. Can you resolve them by merging the latest master into your branch?
You can throw away all the files in docs because I also updated them on our gh-pages branch. The rest should be easy.

@splashsky splashsky merged commit 7dda8d4 into Annoraaq:master Mar 15, 2021
@splashsky splashsky deleted the migrate-to-esbuild branch March 15, 2021 15:00
@splashsky
Copy link
Author

Whoops, accidentally merged this - still new to Git and was just trying to resolve the conflicts lol

Either way, conflicts were fixed and merged into master. 😄

splashsky pushed a commit that referenced this pull request Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to esbuild, clean up repo
2 participants