Implement astTransformer - Compatibility with ts-jest 23.10 #204

wtho · 2018-11-28T00:18:49Z

Closes #195 and closes #201

Breaking Change

This is a breaking change as Configuration Interface changes (see ts-jest v23.10 docs)

This PR is built upon @huafu's #201, kudos to him for setting up everything and pointing out what was still to be done. Transformer and test are also heavily inspired by his examples in ts-jest. Also see the PR for more information.

Implementation Details

Like the Regex, all the transformer does is transform

templateUrl: <PATH> to template: require(<PATH>)
styles: <ANYTHING> to styles: []
styleUrls <ANYTHING> to styleUrls: []

For more information see the comments in the transformer file or #201.

Edge Cases

I had to make some decisions on how strict the astTransformer is, I decided for this, but I am open to discussion:
Works

import { Component as Cmp } from '@angular/core';
@Cmp({
  templateUrl: './some-path.html'
})
class AngularComp { }

Caveat - this also gets transformed

@SomeOtherDecorator({
  templateUrl: './some-path.html'
})
class SomeClass { }

Does not work

const componentArgs = {
  templateUrl: './some-path.html'
}
@Component(componentArgs)
class AngularComp { }

Additional Notes

Run unit tests for the transformer with npm test
The transformer was written in TypeScript. A prepare-script for was added, which compiles it to JS before publishing and after npm install.
preprocessor.js and its unit tests were removed.

wtho · 2018-11-28T00:31:19Z

To make the example-app's tests pass again, I would suggest to remove their execution from the circleci tasks for now and add them after this version got published the first time.

After that the tests will run again referencing jest-preset-angular from package.json avoiding version conflicts and Angular can be bumped to v7.

[Edit: removed irrelevant comment about jest config]

ahnpnl · 2018-12-01T11:43:44Z

hi @wtho, I tried to check out your branch, ran npm run build and then npm pack then install the .tgz file to example app. Then I ran test for the components inside example app, I observed that simple.component.ts test failed with this error

yarn run v1.12.3
$ jest
 FAIL  src/app/simple/simple.component.spec.ts
  ● Test suite failed to run

    Cannot find module 'simple.component.html' from 'simple.component.ts'

      3 | @Component({
      4 |   selector: 'app-simple',
    > 5 |   templateUrl: 'simple.component.html',
        |   ^
      6 |   styleUrls: ['./simple.component.css']
      7 | })
      8 | export class SimpleComponent implements OnInit {

      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:221:17)
      at src/app/simple/simple.component.ts:5:3
      at Object.<anonymous> (src/app/simple/simple.component.ts:8:1)

The same error I saw when running this .tgz file against my company's project. Not all components' tests failed but only a few of them.

README.md

ahnpnl · 2018-12-01T11:47:33Z

@thymikee are you familiar with ast transformation ? I don't have much experience with it.

wtho · 2018-12-01T14:49:28Z

You are correct with the test! I did not know paths like templateUrl: 'page.html' were allowed in angular and have to be transformed before putting them to require.

The transformer now adds ./ if the path does not start with ./, ../ or /. The simple-component test should work now.

The prepare Script is a special one that will automatically run after you npm install without any additional args and before npm publish, so it is not required to call npm run build.

To make the example-app tests pass you have to run:

cd example
npm install

# install version of this PR in example-app, will also get [email protected]
# and build InlineHtmlStripStylesTransformer.js from ts
# (did not know about that, but works! npm ftw!)
npm install wtho/jest-preset-angular#ts-jest-23-10

npm test

I wanted to add this workaround to the CI-Tests as well until next publication, but yarn does not build the package after installed from github directly as npm does, so it would have been more complicated.

I has issues with jest's caching, as I ran the tests long ago, so I run the tests with npm test -- --no-cache to make sure the transformer gets called every time.

Also one more test in the example app fails for me, seems to be related to the Serializers (on Linux, Node v11):
AppComponent > snaps:

-       aaa \$1234
+       aaa $1234

-      \`test'chars""
+      `test'chars""

index.js

circle.yml

__tests__/InlineHtmlStripStylesTransformer.test.ts

jest-preset.js

.gitignore

__tests__/InlineHtmlStripStylesTransformer.test.ts

ahnpnl · 2018-12-01T16:40:53Z

src/InlineHtmlStripStylesTransformer.ts

+            const templatePathStringLiteral: StringLiteral = assignment.initializer;
+            // match if it starts with ./ or ../ or /
+            if (templatePathStringLiteral.text &&
+              !templatePathStringLiteral.text.match(/^(\.\/|\.\.\/|\/)/)) {


I haven't tested this improvement yet but by only reading the codes, it's not so generic to me. Because it's not so sure where the users can put their template, it might be ./ , ../../ or some other places within the project. I think it's better to resolve the path within the project or at least within src/app

True!
I just checked the old regex mechanism, it would also not support templateUrl: 'src/page.html' and transform it to ./src/page.html.
When the astTransformer hits, it only gets config-set injected. Luckily, the ConfigSet object seems to have a method resolvePath, I will see later if this serves our purpose on checking if require will be able to find the file later on.

Ok I looked into the issue a bit and saw the ts-compiler itself (which is also available in the transformer) offers many more path-tools than expected:
toPath, normalizePath, normalizePathAndParts, getDirectoryPath, isRootedDiskPath, isDiskPathRoot, convertToRelativePath, getPathComponents, reducePathComponents, getNormalizedPathComponents, getNormalizedAbsolutePath, getPathFromPathComponents, getRelativePathFromFile, getRelativePathFromDirectory, getRelativePathToDirectoryOrUrl, ensurePathIsNonModuleName, combinePaths, resolvePath, comparePathsCaseSensitive, comparePathsCaseInsensitive, comparePaths, containsPath, extensionFromPath, tryGetExtensionFromPath, getAnyExtensionFromPath, fullTripleSlashReferencePathRegEx, fullTripleSlashAMDReferencePathRegEx, getExternalModuleNameFromPath, getOwnEmitOutputFilePath, getDeclarationEmitOutputFilePath, getSourceFilePathInNewDir, getOutputPathsFor, updateMissingFilePathsWatch, getDefaultLibFilePath

But it feel like it would be an own task to figure out which methods help us to figure out if the file path is relative to the transformed file or relative to the project root using any path-definition inside tsconfig.json.

I quickly checked resolvePath, but that always appends the file to the project root.

Would you be ok with opening a new issue and for now keep the same behavior as the regex did before?

Of course opening another PR for it is always fine by us👌 The only problem is we will need to create a temporary branch for this PR to merge in instead of master. Because master branch at the moment doesn't encounter this issue therefore we want to keep the state like it is right now. Until we solve the issue with this resolve path we can merge the branch of this PR into master. Do you agree @thymikee ?

I disagree, the regex I use, /^(\.\/|\.\.\/|\/)/, does (in combination with moving the string literal and renaming the property assignment to template) the same thing as the regex in master /templateUrl\s*:\s*('|"|)(./){0,}(.*)('|"|)/g, so I see no issue with merging to master for now.

The problem you describe (src/component => ./src/component) has existed before.

I think this is not a reason to hold back this PR.

hmm ok. I hope we make the users happy with this breaking change before they find out this hidden bug :)

Quick Intro AST and the transformer

(Ignore if you know this already)

If you play with a ast-viewer by putting in a simple Angular Component you can get familiar with the AST that TSC will parse form the code. It is a big tree of nested nodes.

Every AST node will have a kind-property, which is a SyntaxKind (all of them listed in typescript.d.ts).

For every SyntaxKind there is a is<KIND>(node)-method, e. g. isStringLiteral(node). I use these in the transformer, e. g. here, and can then safely assume the node is of that kind and has the corresponding fields, so there is no need to check against the number 'manually'.

Once called getMutableClone() on a node it is possible to modify inner nodes. Nodes can also be programmatically created, as explained here in the TS docs. We have to do this in the transformer to e. g. create the require()-call.

Being familiar with this enables you to write custom TSLint rules as well.

wtho · 2018-12-01T23:09:39Z

Thank you for your feedback @thymikee @ahnpnl, feel free to check out the latest changes!

package.json

* Remove obsolete processor which does not work with ts-jest 23.10 * Add AstTransformer to work with the new ts-jest API, that replaces templateUrl with template and a require call and strips off styles from Components for Tests in Jest * Add build commands to repository as transformer is written in TypeScript * Call build in prepare-script to include it in publish * ASTTransformer handles non-relative/non-absolute templateUrl by prefixing them with './'

wtho · 2018-12-02T10:20:46Z

⚠️ Attention

I rebased on top of the updated master (to include #207) and - as all of the commit hashes in this PR changed anyways - took the opportunity to squash all of my commits into one. huafu's commit remained untouched.

ahnpnl · 2018-12-02T11:42:27Z

thanks 👍 Strange CI still failed because one of the snapshots

wtho · 2018-12-02T12:39:20Z

Strange CI still failed because one of the snapshots

Yeah it's odd, I want to figure this out before merging.

wtho · 2018-12-02T13:36:54Z

Ok so this is also a breaking change of ts-jest 23.10, of which they are probably not aware of
cc @huafu

Previously ts-jest used to handle html just calling JSON.stringify(htmlFileContent) in preprocess

now they create a module using result = `module.exports=${JSON.stringify(source)}` in ts-jest-transformer and process it like a file using babel.

This changes the output in the snapshots as (I guess) babel removes the escaping.

How do we handle this?

ahnpnl · 2018-12-02T13:53:08Z

It comes from stringifyContentPathRegex which to keep backward compat of the old option __HTML_TRANSFORM__. Even though the option itself is to keep the existing test working but potentially breaks some snapshot files.

Btw I like the new snapshot more :) the new snapshot actually is more correct than the old one.

wtho · 2018-12-02T21:53:12Z

Ok, so summarizing these things are still open:

I will update the snapshots that test the escape characters
Where will we explain the users the breaking change? There should be a mention of the changes in the config somewhere and a note that the snapshots might have a different outcome. In the README? Or elaborate them more in the Changelog?
Can the transpiled file in .gitignore be kept there for now?

Follow-up issues

Take Serializers to TS and build them as well
Open issue with the bug: templateUrl: 'src/page.html'
Another issue (maybe you have encountered it before): When I run (on the example app) yarn test -t='snap$', the test fails as well. The tests in app.component.spec.ts depend on being run in the order of occurrence as in the file. This is not optimal.

ahnpnl · 2018-12-03T06:57:00Z

Breaking changes should be shortly explained in CHANGLOG and link to this PR. Perhaps in this case of snapshot you can refer it to ts-jest documentation.
I saw the comment from @thymikee about transpile file and convert file in this project to ts file. I guess it's ok to leave it in .gitignore.
The 3rd issue about the test of app component I am not aware of that error. We have a lot of rooms to improve tests in example app.

thymikee · 2018-12-03T10:15:53Z

Breaking changes should be shortly explained in CHANGLOG and link to this PR. Perhaps in this case of snapshot you can refer it to ts-jest documentation.

Yup, plus if there's a migration path, feel free to also add it to the changelog :)

wtho · 2018-12-04T12:13:40Z

I updated the changelog and added a migration guide (ts-jest does apparently not offer one, so I explained the changes to be made in the config).

Btw the snapshots does not differ because of JSON.stringify, but because of the regex ESCAPE_TEMPLATE_REGEX added in #34 is not used anymore. It is not required anymore, as the transformer works on the AST instead of on character-based regexes. I mentioned this in the changelog as well.

Feel free to edit the changelog in any way before the v7 release.

thymikee

This is awesome! Thank you so much on working on this. It means a lot, especially since I have no time for feature development like this anymore.
Left 2 small nits to address, once they're resolved we're gonna merge it and ship as an alpha for testing.

package.json

thymikee · 2018-12-04T12:25:15Z

@wtho let me know if you'd like to help more on the project and become a collaborator just like @ahnpnl who also does tremendous job keeping this project alive.

package.json

wtho · 2018-12-04T13:00:36Z

@wtho let me know if you'd like to help more on the project and become a collaborator just like @ahnpnl who also does tremendous job keeping this project alive.

Yeah, I would love to! Also thank you both for helping me through this PR so far.
It is great you are so quick with responses and giving constructive feedback, this is how open source is fun!

thymikee · 2018-12-04T14:20:45Z

Published this as 7.0.0-alpha.1. If anyone is interested in testing this, feel free to install jest-preset-angular@next :)

ahnpnl reviewed Dec 1, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

thymikee reviewed Dec 1, 2018

View reviewed changes

ahnpnl requested changes Dec 1, 2018

View reviewed changes

wip to make it compatible with ts-jest 23.10 and remove regexp replacers

664716f

ahnpnl reviewed Dec 2, 2018

View reviewed changes

package.json Outdated Show resolved Hide resolved

Update ts-jest dependencies, use injected ts

b63a3c8

ahnpnl approved these changes Dec 2, 2018

View reviewed changes

Use previous file structure, add transformer test

9fce287

thymikee approved these changes Dec 4, 2018

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

Simplify dependencies - remove @types/node

d12728c

thymikee reviewed Dec 4, 2018

View reviewed changes

package.json Outdated Show resolved Hide resolved

Update yarn.lock

ed46803

thymikee merged commit adad842 into thymikee:master Dec 4, 2018

wtho deleted the ts-jest-23-10 branch December 4, 2018 13:14

ahnpnl added this to the 7.0.0 milestone Dec 13, 2018

wtho mentioned this pull request Apr 3, 2019

InlineHtmlStripStylesTransformer is not strict enough #254

Closed

gziolo mentioned this pull request Jul 26, 2019

Test: Collapse passed tests in Travis. WordPress/gutenberg#16755

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement astTransformer - Compatibility with ts-jest 23.10 #204

Implement astTransformer - Compatibility with ts-jest 23.10 #204

wtho commented Nov 28, 2018 •

edited by thymikee

Loading

wtho commented Nov 28, 2018 •

edited

Loading

ahnpnl commented Dec 1, 2018

ahnpnl commented Dec 1, 2018

wtho commented Dec 1, 2018

ahnpnl Dec 1, 2018

wtho Dec 1, 2018

wtho Dec 1, 2018

ahnpnl Dec 1, 2018

wtho Dec 2, 2018

ahnpnl Dec 2, 2018

wtho Dec 2, 2018

wtho commented Dec 1, 2018

wtho commented Dec 2, 2018

ahnpnl commented Dec 2, 2018 •

edited

Loading

wtho commented Dec 2, 2018

wtho commented Dec 2, 2018 •

edited

Loading

ahnpnl commented Dec 2, 2018 •

edited

Loading

wtho commented Dec 2, 2018 •

edited

Loading

ahnpnl commented Dec 3, 2018

thymikee commented Dec 3, 2018

wtho commented Dec 4, 2018

thymikee left a comment

thymikee commented Dec 4, 2018

wtho commented Dec 4, 2018

thymikee commented Dec 4, 2018

Implement astTransformer - Compatibility with ts-jest 23.10 #204

Implement astTransformer - Compatibility with ts-jest 23.10 #204

Conversation

wtho commented Nov 28, 2018 • edited by thymikee Loading

Breaking Change

Implementation Details

Edge Cases

Additional Notes

wtho commented Nov 28, 2018 • edited Loading

ahnpnl commented Dec 1, 2018

ahnpnl commented Dec 1, 2018

wtho commented Dec 1, 2018

ahnpnl Dec 1, 2018

Choose a reason for hiding this comment

wtho Dec 1, 2018

Choose a reason for hiding this comment

wtho Dec 1, 2018

Choose a reason for hiding this comment

ahnpnl Dec 1, 2018

Choose a reason for hiding this comment

wtho Dec 2, 2018

Choose a reason for hiding this comment

ahnpnl Dec 2, 2018

Choose a reason for hiding this comment

wtho Dec 2, 2018

Choose a reason for hiding this comment

Quick Intro AST and the transformer

wtho commented Dec 1, 2018

wtho commented Dec 2, 2018

⚠️ Attention

ahnpnl commented Dec 2, 2018 • edited Loading

wtho commented Dec 2, 2018

wtho commented Dec 2, 2018 • edited Loading

ahnpnl commented Dec 2, 2018 • edited Loading

wtho commented Dec 2, 2018 • edited Loading

ahnpnl commented Dec 3, 2018

thymikee commented Dec 3, 2018

wtho commented Dec 4, 2018

thymikee left a comment

Choose a reason for hiding this comment

thymikee commented Dec 4, 2018

wtho commented Dec 4, 2018

thymikee commented Dec 4, 2018

wtho commented Nov 28, 2018 •

edited by thymikee

Loading

wtho commented Nov 28, 2018 •

edited

Loading

ahnpnl commented Dec 2, 2018 •

edited

Loading

wtho commented Dec 2, 2018 •

edited

Loading

ahnpnl commented Dec 2, 2018 •

edited

Loading

wtho commented Dec 2, 2018 •

edited

Loading