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 possibility to apply comments for coverage settings in transforms #153

Merged
merged 8 commits into from Aug 9, 2017
Merged

Add possibility to apply comments for coverage settings in transforms #153

merged 8 commits into from Aug 9, 2017

Conversation

Hobart2967
Copy link
Contributor

see #152

@Hobart2967
Copy link
Contributor Author

see #152

@Hobart2967
Copy link
Contributor Author

#152

@erikbarke
Copy link
Collaborator

This is a much better approach than the one I had in mind, it's simple yet effective 😃
However, it slightly breaks the api contract since the dirty flag now is ignored in some cases. I think the callback should have a new, third parameter: (error: Error, dirty: boolean, transpile: boolean)

  • the transformed code gets bundled only if the dirty flag is set to a truthy value, it doesn't matter if the source code or the transpiled code has been transformed. Data from the callback should be ignored if the dirty flag is set to a falsy value.
  • the transformed code only gets recompiled if the flag transpile is set to a truthy value. To avoid breaking changes, this flag is optional and defaults to true.

So, in your case, the callback would be error = undefined, dirty = true, transpile = false.

Also, for the sake of consistency in the Transforms API I think the emitOutput property in the context object should be renamed to transpiled and its values should be copied to a new object, otherwise any changes would be made directly on the original object, bypassing the dirty flag.

If you don't want to wait for the ci servers to give you a slap on the wrist, just run the linter and the tests locally, the commands are in .travis.yml (*nix) and appveyor.yml (windows) in the root of the project.

My apologies for creating a wall of text here, I'm posting from my phone so it's hard to use the code review tools...

When this pr is done it's gonna be a really nice addition to the Transforms API 🎉

@erikbarke erikbarke modified the milestones: 3.0.5, 3.0.6 Jul 25, 2017
@erikbarke
Copy link
Collaborator

Are you still working on this PR? If not, I'll merge it as-is and make the suggested modifications myself, just let me know!

@erikbarke erikbarke changed the base branch from master to next August 9, 2017 05:50
@erikbarke erikbarke merged commit fc88dff into monounity:next Aug 9, 2017
@erikbarke
Copy link
Collaborator

Hey, I've modified the API, it looks like this now:

karmaTypescriptConfig: {
    bundlerOptions: {
        transforms: [
            function(context, callback) {
                if(context.ts) {
                    context.ts.transpiled = "\n/* istanbul ignore next */\n" + context.ts.transpiled;
                    return callback(undefined, true, false);
                }
                return callback(undefined, false);
            }
        ]
    }
}

The javascript code is in context.ts.transpiled and to avoid a recompile, call the callback with the third flag set to false: callback(undefined, true, false);.

@Hobart2967
Copy link
Contributor Author

(y) nice! thanks mate!

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

2 participants