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

Support for tests requiring strict mode enabled in npm package @bazel/karma #922

Closed
KrauseStefan opened this issue Jul 17, 2019 · 1 comment · Fixed by #1310
Closed

Support for tests requiring strict mode enabled in npm package @bazel/karma #922

KrauseStefan opened this issue Jul 17, 2019 · 1 comment · Fixed by #1310
Labels
cleanup Tech debt, resolving it improves our own velocity
Milestone

Comments

@KrauseStefan
Copy link
Contributor

🚀 feature request

I am currently in the process of adding Bazel support to our AngularJs based build, in preparation for an upgrade to Angular.
I ran into this problem during this.

Relevant Rules

Applies to ts_web_test_suite and karma rules.

Description

We have some older code that depends on exceptions being thrown when you try to modify an object that has had Object.freeze() applied to it.
It is important that we don't just get a silent error in those cases.

I understand that you cannot always simply keep the 'use strict'; annotation when concatenating files. It should however be okay when a function scope is applied around the code and the 'use strict'; annotation.
The default behaviour probably should be that it is removed at least for third party.

I ask for a supported solution to having strict mode enabled where needed.

The removal happens on line 49 in packages/karma/index.ts

Describe the solution you'd like

I suggest that dependencies are treated different from local source and are allowed to work with strict mode, ideally you would be able to specify the behaviour per dependency as well.

Describe alternatives you've considered

Currently we are using a workaround by having two 'use strict'; at the top of the file. Only one of them is removed, I would however like to use a supported method.

(function () {
  'use strict';
  'use strict'; // Workaround bazel removing 'use strict'

  describe('makeReadOnly', function() {
@alexeagle
Copy link
Collaborator

Hmm, some research on this one:

We stripped the use strict since it was introduced way back in
https://github.com/bazelbuild/rules_typescript/pull/72/files#diff-1bd7116f44b6aa59fa877330a0bf22f5

I think we do this because of concatjs, but in May 2018 the 'use strict' was removed from google's copy of this code (googlers: cl/198411444)

So the right thing is to always turn on 'use strict' matching our google-internal. We should try to do it before 1.0 since it's a breaking change. Ideally we should open-source our karma config so that it stays in sync with what Google has in the future too.

@alexeagle alexeagle added cleanup Tech debt, resolving it improves our own velocity package:karma labels Sep 23, 2019
@alexeagle alexeagle added this to the 1.0 milestone Sep 23, 2019
alexeagle added a commit to alexeagle/rules_nodejs that referenced this issue Oct 28, 2019
Replace the eval() with <script> tags for each script, so that they still create global variables

Similar to g3 cl/198411444

Fixes bazel-contrib#922
alexeagle added a commit that referenced this issue Oct 28, 2019
Replace the eval() with <script> tags for each script, so that they still create global variables

Similar to g3 cl/198411444

Fixes #922
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Tech debt, resolving it improves our own velocity
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants