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

Refactor with ES6 class syntax #373

Closed
wants to merge 23 commits into from
Closed

Conversation

mxmason
Copy link
Contributor

@mxmason mxmason commented Jul 1, 2022

Summary

In addition to switching to class syntax, this PR:

  • adds all compiled JavaScript files to .gitignore, so only the source files are committed to version control
  • gets rid of the create method, as the constructor serves the same purpose and create is not advertised externally
  • adjusts the rollup config to output transpiled UMD files and untranspiled ESM files

It's worth noting that, while switching to class syntax makes the ESM files slightly smaller, the transpilation of class syntax to ES5 makes the UMD output slightly bigger.

Size of UMD output after this migration: 1.5kb (+1kb)
Size of ESM output after this migration: 1.2kb (
-1kb)

@KittyGiraudel
Copy link
Owner

Yaaaay, alright, let’s get down to business! First of all, thank you so much for taking the time to bring improvements, I truly appreciate it.

I think I would like if we could break down the 3 items into 3 separate PRs:

  1. One to remove the dist files from the repository. I honestly can‘t remember why there were here in the first place. I think at some point I thought people might want to copy/paste the code directly? But that’s probably an outdated approach. What do you think?
  2. One to move the create code into the constructor. Super good idea, I like it!
  3. One to adjust the Rollup configuration.

Do you think that‘s possible? This way we can merge them in isolation and the changelog is a little nicer. :)

@mxmason
Copy link
Contributor Author

mxmason commented Jul 1, 2022

I can do that. I think items (2) and (3) could be in the same PR, because in my mind, adjusting the rollup config is a necessary response to migrating to class syntax.

@KittyGiraudel
Copy link
Owner

Makes sense. Although we could do drop the create method before switching to the class syntax?

@mxmason
Copy link
Contributor Author

mxmason commented Jul 1, 2022

Sure! To confirm my understanding, the sequence of PRs is:

  1. Remove output files from git history
  2. Remove create
  3. Migrate to es6 and update the rollup config
  4. (Not in this PR) Migrate to TypeScript source code

Is that right?

For what it's worth, I think you could delay cutting a release until I complete the TypeScript migration because we aren't meaningfully changing the user-facing API, but of course I defer to you

@KittyGiraudel
Copy link
Owner

Yes, that sounds fair! I think we can bundle all this in v8 for safety. And the order sounds right. I’m also super happy to support any way I can. I don’t want to abuse your time. 😊 ✨

@mxmason
Copy link
Contributor Author

mxmason commented Jul 1, 2022

Don't worry about it! I can get started in the next few minutes. As to your question about whether folks would want to copy the compiled code: Maybe the readme could link to some CDNs that host the code? I don't think there's a huge need for direct access to the compiled code these days, though?

@KittyGiraudel
Copy link
Owner

Yeah, you’re probably right. I guess we could link to Skypack: https://www.skypack.dev/view/a11y-dialog

@KittyGiraudel
Copy link
Owner

Closing in favor of #374, #375, #376.

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.

2 participants