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

feat: no production assert tslint rule #894

Merged
merged 3 commits into from Dec 20, 2018

Conversation

jodarove
Copy link
Contributor

@jodarove jodarove commented Dec 19, 2018

Details

We should strip asserts when compiling the framework code for production environment.
For that the assertion code should be wrapped in: if (process.env.NODE_ENV === 'production') {}.

This pr adds a tslint rule to ensure this.

Does this PR introduce a breaking change?

  • Yes
  • No

We should strip asserts when compiling the framework code for production environment.
For that the assertion code should be wrapped in: if (process.env.NODE_ENV === 'production') {}.

This is the simple, but good enough implemetation for the code we have.
@jodarove jodarove mentioned this pull request Dec 19, 2018
2 tasks
@jodarove
Copy link
Contributor Author

@pmdartus @ekashida @caridy : your feedback in #889 is addressed in this pr. thanks!

@jodarove jodarove force-pushed the jodarove/no-production-assert-rule branch from 26d9407 to f20c59a Compare December 19, 2018 00:37
Cause they are checked before calling.
@jodarove jodarove force-pushed the jodarove/no-production-assert-rule branch from f20c59a to 972feba Compare December 19, 2018 06:54
Copy link
Member

@pmdartus pmdartus left a comment

Choose a reason for hiding this comment

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

Small style detail. But except that LGTM

packages/@lwc/engine/tslint.json Show resolved Hide resolved
@pmdartus
Copy link
Member

Another approach to tackle this issue would be to lint the generated production code instead of the dev code. It would alleviate the need to add tslint inline comments for the functions that we know will be stripped in PROD (very fragile).

@jodarove Thoughts?

Copy link
Contributor

@caridy caridy left a comment

Choose a reason for hiding this comment

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

Ok, I'm fine with this as it is!

@jodarove
Copy link
Contributor Author

Another approach to tackle this issue would be to lint the generated production code instead of the dev code. It would alleviate the need to add tslint inline comments for the functions that we know will be stripped in PROD (very fragile).

@jodarove Thoughts?

@pmdartus i agree that we should add a check (some how) in the generated production code to verify no asserts are leaking before it actually gets to prod, which is a late.

That being said, this pr addresses a different issue, is meant to give an early feedback to devs that they might be leaking asserts to prod, trusting that the ci lint phase will fail and the build process of the prod scripts will do the work stripping what is wrapped in the if condition.

Copy link
Member

@ekashida ekashida left a comment

Choose a reason for hiding this comment

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

Curious where the tests went? Were they difficult to put into place after moving the rules inside engine?

packages/@lwc/engine/tslint.json Show resolved Hide resolved
@jodarove
Copy link
Contributor Author

jodarove commented Dec 19, 2018

@pmdartus @caridy @ekashida : before merging, pick your poison: this way or #898

@jodarove
Copy link
Contributor Author

@ekashida you dont miss one eh? :)

I was having an issue with jest generating the coverage (without coverage is fine), in order to run the tests in the script/rules folder, a new entry needs to be added in roots from jest.config.js, adding this was causing an exception when generating the coverage. this investigation will take time, though i can add the tests from tslint if you feel more confident.

All lint rules are defaulted to error.
@jodarove jodarove merged commit 1e1aa91 into master Dec 20, 2018
@jodarove jodarove deleted the jodarove/no-production-assert-rule branch December 20, 2018 00:57
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.

None yet

4 participants