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

Tasks cleanup #44

Closed
resir014 opened this issue May 11, 2017 · 22 comments
Closed

Tasks cleanup #44

resir014 opened this issue May 11, 2017 · 22 comments

Comments

@resir014
Copy link
Member

resir014 commented May 11, 2017

Our current gulpfile/tasks pipeline has grown too much it might be overwhelming for new users to get started with the starter kit. Rewriting the entire tasks system should be our main priority before upgrading to TS 2.3.

  • Make each features of the starter kit (e.g. testing, etc.) optional (yo generator perhaps?)
  • Improve the available guides to be noob-friendly (assume users have no prior knowledge of JS)
  • Consider ditching gulp for other better alternatives (pure npm tasks/brunch) (might have to ditch the flattened directory option)
  • Look into using brunch (idea: @bryanbecker) We might just use pure npm tasks + Webpack after all.
@kotarou
Copy link
Member

kotarou commented May 11, 2017

A setup script that allowed selection of options would be excellent, e.g.

What style of linting do you want? (S)trict, (l)oose, or (n)one?
Do you wish to remove comments?
Do you want your TS to be compiled strictly?
Should testing and code-coverage systems be installed? (Y)es, (t)ests only, (n)o...
etc.

Even though these examples may be single line changes, it can be intimidating to find where to make them, especially while also dealing with the huge number of new things to install.

@bryanbecker
Copy link
Member

I know this is about "clean-up", but if we are adding configurable options, I think the profiler should be considered as well

@resir014
Copy link
Member Author

@bryanbecker We could do that, yes.

Speaking of which, do we want to make the logger (located in src/lib/logger) to be a separate module as well? If yes, I could ask @ezolenko (he originally wrote it) if he wants to do that.

@kotarou
Copy link
Member

kotarou commented May 11, 2017

I think that having both the logger and profiler as selectable modules is an excellent idea.

@bryanbecker
Copy link
Member

Perhaps a few question that should be decided first are:

  • Do we assume the user has starting knowledge of typescript?
  • If not, does this project provide a guide?
  • (same two above questions, but for Screeps)

@resir014
Copy link
Member Author

resir014 commented May 11, 2017

@bryanbecker

Do we assume the user has starting knowledge of typescript?

I think we should really start assuming that the users have little to no knowledge of TypeScript, since we've already seen quite a few Screeps newcomers would come across this project (sometimes with even no prior knowledge of JS), wanting to know what this project is all about.

In which case...

If not, does this project provide a guide?

The readme that we currently have is the only guide that we have right now, and I have to admit that I'm not that good at writing guides.

We should provide the bare essentials to get started on the readme, and use the currently-empty wiki as a community-maintained effort to provide some noob-friendly guides on how to get started on the starter kit itself, or in general (TypeScript, Screeps, etc.)

@kotarou
Copy link
Member

kotarou commented May 11, 2017

I agree, we should assume very little knowledge.

I am happy to write a guide for the wiki here, but I wonder - should we instead (also?) provide a guide on TS for the actual screeps website?

@bryanbecker
Copy link
Member

I agree that we should approach it as a tool for beginners.

I quite like the logger. It was useful to me as an example on how to use memory to store config data. It would be disappointing if it was moved to a separate where it may be ignored by beginners.

Also, might want to keep the wiki ts-related. Considering the Screeps' wiki pays real $$ for contributions, the wiki here may not get too much use for Screeps content


On a different topic, does the current set-up support webpack's "dead code elimination"?

If so, we could take a much more configurable and user-friendly approach for some things by using build toggles

@resir014
Copy link
Member Author

resir014 commented May 11, 2017

On a different topic, does the current set-up support webpack's "dead code elimination"?

Not at the moment, but it's a useful feature to have.

@bryanbecker
Copy link
Member

bryanbecker commented May 11, 2017

Looking at brunch now... it provides "skeleton" features, which would make this project nicer

https://github.com/brunch/brunch-guide/blob/master/content/en/chapter02-getting-started.md


Update: I no longer think brunch is a good choice for this project (see below). I'm looking into seeing how much we can get done with webpack alone

From version 1.8.2 up to current version, this plugin may report TypeScript errors that you are not expecting. This is due to the fact that this plugin compiles each file separately in isolation, and doesn't take advantage of the full TypeScript project. As such there are some errors which may appear which are false positives.

Starting in 1.8.3 you could add an ignoreErrors property the plugin config object in the brunch-config file. This was an array of error numbers to ignore. Starting in 2.0.1, you can ignore all TypeScript errors by setting ignoreErrors to true (this was broken in 2.0.0). Setting it to an array still works as before.

We are hoping to support the full language service, at least for brunch build at some point, but until then, we recommend that you add tsc --noEmit to your test script or build script to catch proper errors within your project.

Just to note that this shouldn't affect any TypeScript support your editor/IDE provides, which should also allow you to identify real errors.

@resir014
Copy link
Member Author

@bryanbecker Cool. I'm going to start experimenting on removing gulp this weekend as well.

@bryanbecker
Copy link
Member

If anyone can provide a list of exactly what tasks the old gulpfile did, I think that would help in the rewrite

@resir014
Copy link
Member Author

resir014 commented May 12, 2017

@bryanbecker Here's what I could gather for now:

  • The default task basically runs a watch on src/**/*.ts, then runs the build command on file change.
  • build runs the whole compiling process and uploads the bundled/flattened code to the screeps server, or to the local folder (for private servers).
    • It accepts arguments, like --target (dev|prod)
    • the entry point for the whole task is compile. It runs either compile-bundled or compile-flattened, depending on config.json
    • The task pipeline includes, in order:
      • gitRevisions - this was for the sourcemaps/logger. I forgot what it does exactly, I didn't write it. (Only run on compile-bundled)
      • lint and clean are both run in parallel at this point. Which is pretty much self-explanatory.
      • upload runs the upload to the screeps server using our in-house gulp-screeps-upload library.
  • test - runs lint first, then executes mocha through gulp-shell.

Let me know if you have any more questions!

@bryanbecker
Copy link
Member

bryanbecker commented May 12, 2017

OK I have to ask for some help on my PR. It's almost ready to go, but I can't figure out why the module being exporting and uploaded is not being recognized by screeps. If anyone has any ideas, I would love to hear. solved

Also, I'm going to leave testing integration for someone else to do, but it should be fairly straightforward

@bryanbecker
Copy link
Member

@resir014 OK, I think the PR is finished and documented (with exception of github/gitlab logging documentation). Let me know what you think. A few things of note (copied from my PR comment):


Testing is not implemented whatsoever. I'm leaving that task to someone else who wants to test their screeps code (ha). I imagine it's extremely straightforward to set up, though.

The webpack version of the source map / git interaction can not automatically pick up the repository path, as it did before. The path now needs to be manually set in the config. There are two large "TODO"s in the Readme where documentation on how to accomplish this needs to be added. Since I don't use this feature, I'm not sure how it should be set.

The screeps-webpack-plugin is set to install from my fork, pending this PR: langri-sha/screeps-webpack-plugin#28

@resir014
Copy link
Member Author

resir014 commented May 15, 2017

@bryanbecker re: testing - I've done some quick investigations to testing and found this file to be the culprit: https://github.com/screepers/screeps-typescript-starter/blob/master/test/mock/game.ts

IIRC, what that file does is that it mocks the game constants located here We simply copied it over and saved it as a .ts file, and made no alterations to types, etc. for us to be able to update the constants file easily whenever it gets updated.

When I run the tests on strict mode, it seems to be giving out noImplicitThis errors:
https://www.typescriptlang.org/docs/handbook/functions.html#this

image

That's what the TypeScript compiler kept complaining about whenever you have strict mode enabled on TS while the tests were run, resulting the tests to fail. If we can find a way around this, the tests should run fine.

@bryanbecker
Copy link
Member

What's the right way to go here?

Does the code need to fixed, or does the error need to be suppressed?

I guess, that's asked "is this working as intended?" or a false error?

@resir014
Copy link
Member Author

@bryanbecker The thing is, if strict mode is disabled, the tests run normally again...

I think I'll see if I could fix the compile errors easily. If not, then we'll have to rewrite the entire testing framework. I'm looking into magical-mock, and it seems promising, we could use it.

@bryanbecker
Copy link
Member

bryanbecker commented May 17, 2017

I took another look at those errors.

I'm pretty sure the thrown error is working as intending. Those declarations don't look like they meet strict standards.

ex:

  // mock/game.ts
  BODYPARTS_ALL: [
    this.MOVE,
    this.WORK,
    this.CARRY,
    this.ATTACK,
    this.RANGED_ATTACK,
    this.TOUGH,
    this.HEAL,
    this.CLAIM,
],

Looks like you have a few ways to type this: microsoft/TypeScript#14141

a lazy way that might work is to just change it to:

const gameConsts: ThisType<any> = {

it's still typed as any, but now it's not implicit

@resir014
Copy link
Member Author

@bryanbecker I just tried using ThisType<any>, still gave the same error.

@bryanbecker
Copy link
Member

Besides testing support, is anything left with this issue (perhaps a cleaning of the docs)?

Maybe close it and reopen a more specific issue referencing this one?

@resir014
Copy link
Member Author

resir014 commented May 25, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants