[go: up one dir, main page]

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

Improving the published package #39

Open
blacha opened this issue May 9, 2019 · 7 comments
Open

Improving the published package #39

blacha opened this issue May 9, 2019 · 7 comments

Comments

@blacha
Copy link
Contributor
blacha commented May 9, 2019

I was looking at using this library for a project I am working on, however the published package makes it kind of difficult.

Would you accept a pull request to update the build process?

Key areas of improvement:

  • Expose ES6 modules
  • Expose Typescript definition files
  • Improving type definitions.
  • Required dependencies as dependencies (eg Victor)

Also running the build in the CI system would be helpful especially if it had some sort of formatting (prettier/other) and linting (eslint/tslint ?) configuration.

@demipixel
Copy link
Owner

Yes, I would accept that. I remember having to do something a little hacky to access the TypeScript definitions, so I can understand the annoyance. Unfortunately I don't think Victor has type definitions.

@blacha
Copy link
Contributor Author
blacha commented May 9, 2019

Victor is quite old, it does actually cause a few headaches.. eg doesn't expose a default on its module exports.

To get it to compile properly, I think you need to add a esModuleInterop into the tsconfig.json.

Someone has also made types for it, in @types/victor

I have made a start on a branch https://github.com/blacha/factorio-blueprint/tree/chore/adding-build-pipeline

I also don't really have any preferences on linting/formatting.. So if you have any strong opinions on tabs vs spaces, single vs double quotes I can change it.

@demipixel
Copy link
Owner

I would say just don't change a lot (e.g. Keep spacing as 2, keep { on same line as functions/ifs, etc).

I'm not attached to Victor, but I'm not sure if it's the best idea to write our own implementation (since we want to do more than just store the value—we add/subtract from it, multiply, clone, etc). I've looked a little but I can't seem to find any alternatives. Not sure exactly what to do.

@blacha
Copy link
Contributor Author
blacha commented May 10, 2019

Awesome, My goal is not to change anything if i can avoid it, Just to make sure its standard across all the files so my editor doesn't keep auto-formatting things causing my commits to get too large.

By just adding a base prettier config, it doesn't really change very much, it is mostly just multi lining long statements, I was going to look at making the default line length longer to reduce the amount of changes,

Here is the base commit so far: https://github.com/blacha/factorio-blueprint/commit/1be1c58804f23f6c462503318baec75545897f20 This is a work in progress, I am looking to reduce the amount of changes.

Its mostly just removing excess commas, adding/removing ; and making long lines multiline. Most of the code that was multi lined could be adjusted to be back on one line.

Victor seems fine as a library, just needs a little bit of work to get it going in Typescript/nodejs land, I have a build working with nodejs compiled as commonjs modules. So might be able to give you a couple PRs tomorrow.

I will split them into a format/linting PR + the node build PR

@slikts
Copy link
slikts commented May 12, 2019

Its mostly just removing excess commas

Trailing commas are a good feature that makes editing easier and reduces diff churn, and Prettier allows enabling them in its config, and it only has them off by default for compatibility reasons that aren't relevant here.

@blacha
Copy link
Contributor Author
blacha commented May 13, 2019

Trailing commas are a good feature that makes editing easier and reduces diff churn, and Prettier allows enabling them in its config, and it only has them off by default for compatibility reasons that aren't relevant here.

I am neither for or against trailing commas. Most of the codebase does not have trailing commas.

So to reduce the size of the PR I left them off.

@slikts
Copy link
slikts commented May 13, 2019

So to reduce the size of the PR I left them off.

MR size is a legit point, but removing the existing trailing commas, referring to them as "excess", and setting up automation that enforces their removal isn't a neutral action.

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

No branches or pull requests

3 participants