pass fixture files through the postHTML transformer code #30681
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
erwinmombay
force-pushed
the
fix-posthtml-transformers-backup
branch
from
a886b65
to
cec9fde
Compare
estherkim
reviewed
Oct 19, 2020
| } | ||
| } | ||
|
|
||
| async function integration() { | ||
| buildNewServer(); | ||
| htmlTransform = require('../server/new-server/transforms/dist/transform') |
There was a problem hiding this comment.
Where does this come from? Is it something that buildNewServer() builds?
There was a problem hiding this comment.
buildNewServer() generates this file from the TypeScript transforms.
There was a problem hiding this comment.
@erwinmombay I just saw this old review comment thread in relation to #32865 (comment). I think this should be moved to server.js. The integration test runner shouldn't have to worry about server startup logic.
erwinmombay
force-pushed
the
fix-posthtml-transformers-backup
branch
from
b043349
to
bb0f659
Compare
…cript-transform.ts each module should do 1 thing and 1 thing only. currently the module transformer also did a src/url transformation. this caused weird mismatches when they are ran together. - added looseScriptSrcCheck to allow for non cdn domains. This will allow us to transition from the bad fixture files to having all html files be valid AMP HTML by default.
Co-authored-by: Caroline Liu <[email protected]>
Co-authored-by: Caroline Liu <[email protected]>
erwinmombay
force-pushed
the
fix-posthtml-transformers-backup
branch
from
bb0f659
to
61e77f5
Compare
erwinmombay
force-pushed
the
fix-posthtml-transformers-backup
branch
from
61e77f5
to
474c6cb
Compare
|
@estherkim PTAL |
estherkim
approved these changes
Oct 20, 2020
| } | ||
| } | ||
|
|
||
| async function integration() { | ||
| buildNewServer(); | ||
| htmlTransform = require('../server/new-server/transforms/dist/transform') |
There was a problem hiding this comment.
nit: Use SERVER_TRANSFORM_PATH from typescript-compile.js
There was a problem hiding this comment.
hope you don't mind ill follow it up in #28336
There was a problem hiding this comment.
@erwinmombay Can we also fix this as part of #32865 (comment)?
ed-bird
pushed a commit
to ed-bird/amphtml
that referenced
this pull request
Dec 10, 2020
…30681) * make it so that src transformation only happens strictly in scripts/script-transform.ts each module should do 1 thing and 1 thing only. currently the module transformer also did a src/url transformation. this caused weird mismatches when they are ran together. - added looseScriptSrcCheck to allow for non cdn domains. This will allow us to transition from the bad fixture files to having all html files be valid AMP HTML by default. * temp * temp * add transform back to integration * fix html * fix integration * temp * temp * make transformers have a loose mode to be more forgiving on script src * add more tests and fix bug for extention retention * allow for mjs files to be served by the test server * add glob to load mjs files in karma server * lint fixes * lint html * break up this PR * fix bad error formatting * apply recs * Update build-system/server/new-server/transforms/transform.ts Co-authored-by: Caroline Liu <[email protected]> * Update build-system/server/new-server/transforms/utilities/option-set.ts Co-authored-by: Caroline Liu <[email protected]> * fix other locations of "FORTESTING" * fix esm and non esm tests to be green * use writeFileSync instead of async method. seems to be a race * guarantee directory exists first * skip failing IE test temporarily * applied recs * temp "only" * dont reroute to max json files * change error to warning * fix max builds and integration tests against max files * revert bad rebase * fix all urls to be valid for js resources * removing debugger * re-run broken IE test Co-authored-by: Caroline Liu <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
we want our example and fixture files to be "valid html" files by default on disk, meaning the url's need to point to the cdn and it passes the validator.
For these files to be usable for tests we then need to pass them through our html transformers to change the url's and do modifications such as: