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

add tslint block #92

Merged
merged 10 commits into from
Jan 25, 2017
Merged

add tslint block #92

merged 10 commits into from
Jan 25, 2017

Conversation

jvanbruegge
Copy link
Contributor

for all typescript linting needs :)

@andywer
Copy link
Owner

andywer commented Jan 19, 2017

Awesome! :)

I am on a train right now and writing from my iPhone, but if I should forget to write a main contributors list please tell me. You are definitely on it 😉

@andywer
Copy link
Owner

andywer commented Jan 19, 2017

Feel free to add the block to the list in the main README as well :)

@jvanbruegge jvanbruegge changed the title added initial tslint block config add tslint block Jan 20, 2017
@andywer
Copy link
Owner

andywer commented Jan 20, 2017

I applied two minor changes. Added an npm version badge (which will work as soon as it's published) and I replaced the require('webpack') by using context.webpack.

@jvanbruegge
Copy link
Contributor Author

Is the options plugin also working for webpack 1? Maybe I should add an e2e test

@andywer
Copy link
Owner

andywer commented Jan 20, 2017

I was going to write this just now 😉 Could you please use the tslint block in the TypeScript e2e tests?

And of course, the LoaderOptionsPlugin will probably work with webpack 2 only... We might just copy the approach used in the postcss block for now.

But that reminds me of something... I wanted to add the webpack version to the context. Need to open an issue for that as well.

@jvanbruegge
Copy link
Contributor Author

if we have the version we dont need to split, becaude then the object is dumped in the webpack config

@andywer
Copy link
Owner

andywer commented Jan 20, 2017

But for webpack 1.x we will need to add the tslint config directly to the webpack config object, for webpack 2 we need to use the LoaderOptionsPlugin. So we need the version to distinguish which way to go, but it must be able to do both, right?

@andywer andywer added this to the 0.4.0 milestone Jan 20, 2017
@jvanbruegge
Copy link
Contributor Author

jvanbruegge commented Jan 20, 2017

the block yes, but thats simple

@jvanbruegge
Copy link
Contributor Author

I can do this when #96 is merged

@andywer
Copy link
Owner

andywer commented Jan 20, 2017

Sure, that would be the easiest solution :)

We just have to add a peer dependency '@webpack-blocks/core': '>= 0.4.0' to the tslint block then.

@jvanbruegge
Copy link
Contributor Author

I rebased on #96, but the tslint-loader does not complain even if it should (mixed semicolon style in app.ts). Works in both webpack version now

@jvanbruegge
Copy link
Contributor Author

The block is now completely working, you can test it by adding a semicolon in the app.ts of the end2end test. It will break the build

@jvanbruegge
Copy link
Contributor Author

@andywer this should be finished now, can you take another look?

@andywer
Copy link
Owner

andywer commented Jan 24, 2017

I think this feature branch is quite broken... There are still some remains of code that is not part of the tslint block, but not part of the master branch either.

Have a look at the changes of webpack/index.js and webpack2/index.js.
You merged some other feature branch into this one before, I think that in combination with the latest rebasing broke it. Sorry 🙈

@jvanbruegge
Copy link
Contributor Author

You are right, one wrong commit sneaked in. I deleted it.

loaders: [ 'tslint-loader' ]
})

const module = context => (context.webpackVersion.major === 1 ? {
Copy link
Owner

Choose a reason for hiding this comment

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

standard usually requires you to put a single parameter of an arrow function into parenthesis. Maybe the linting of this code does not work? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I remember putting a semicolon there, and it was cought. Can check later

const module = context => (context.webpackVersion.major === 1 ? {
preLoaders: [ loader(context) ]
} : {
loaders: [ Object.assign({}, loader(context), {
Copy link
Owner

Choose a reason for hiding this comment

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

Might be easier readable as loaders: [ loader(context, { enforce: 'pre' }) ]. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

(2nd param for loader(), will be merged into the loader object using Object.assign())

@jvanbruegge
Copy link
Contributor Author

I tested it, standard is indeed run on the index.js file. Adding a semicolon throws an error.
I also updated the loader function, indeed it makes it a lot more readable

@andywer
Copy link
Owner

andywer commented Jan 25, 2017

Cool 👍

And I just recognized they relaxed the arrow-function-must-have-parenthesis rule in standard 😉 (standard/standard#414)

@andywer
Copy link
Owner

andywer commented Jan 25, 2017

Just changed a line in favor of more consistent code style. We should stick to one style (single-param arrow functions have parenthesis or they don't) at least within the same file 😉

@jvanbruegge
Copy link
Contributor Author

Ok

@andywer
Copy link
Owner

andywer commented Jan 25, 2017

Off-topic: Since you live in Munich... Do you know https://twitter.com/munichjs? :)

@jvanbruegge
Copy link
Contributor Author

No, but it looks very interesting, especially this meetup at the google office

@andywer
Copy link
Owner

andywer commented Jan 25, 2017

Yeah, the people attending it seemed quite interesting as well. Just stumbled across and thought you might have already been there.

@andywer andywer merged commit bbcda3f into andywer:master Jan 25, 2017
@andywer
Copy link
Owner

andywer commented Jan 25, 2017

Published!

@jvanbruegge
Copy link
Contributor Author

yay 🎉 🎉 🎉

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