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

Update tooling #77

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

Dan-Shields
Copy link
Contributor

@Dan-Shields Dan-Shields commented Apr 10, 2021

  • Moves away from deprecated/unsupported versions of various libraries.
  • Removes a bunch of unnecessary dependencies and scripting left over from the initial template.
  • Configures babel for only the versions of chromium we care about.
  • Applies eslint's recommended rules, while trying to keep the same code style as before.

React version is unchanged as its still supported and would require way too much work to be worth it.

If you're wondering why, I'm planning to use this plugin as a basis for an in-house extension at work, and before I get started it'll be nice to have more up-to-date foundations.

EDIT: This will drop support for Node 8 and earlier when developing.

@Dan-Shields
Copy link
Contributor Author

Also something I've tried and seems to work well is changing the ExtendScript .jsx files to .jsxinc. This doesn't change the functionality whatsoever, but makes it more clear whether a file is ExtendScript, or a react JSX file. It's minor so not committed yet, but I'd be interested in your thoughts @bodymovin

@bodymovin
Copy link
Owner

hey thanks so much for all the improvements!!
I'm trying the branch locally to make sure everything is working fine.
As soon as it's ready, I'll merge everything.
Please don't update the PR for now so I don't have to review it all over again :)
Regarding the file renaming, it makes sense. I think it can be done in another PR.

@Dan-Shields
Copy link
Contributor Author

Sounds great, let me know if you find anything that you think needs changing!
Sorry if I made you restart the review, I had some uncommitted changes but everything's there now 👌

@bodymovin
Copy link
Owner

I did a build and it didn't work. I'll have to look into it, but has it worked for you?

@Dan-Shields
Copy link
Contributor Author

I did a build and it didn't work. I'll have to look into it, but has it worked for you?

Apologies, I didn't properly test the prod build since changing the webpack config, public path needed to be set differently. I've updated it and tested and it seems to be working now :)

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