fix gulp integration and server so that it can work with mjs files #30292
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
branch
4 times, most recently
from
0f5ae48
to
ce5d37c
Compare
erwinmombay
changed the title
erwinmombay
force-pushed
the
fix-posthtml-transformers
branch
3 times, most recently
from
c40812e
to
d3751c2
Compare
xiexr151e
reviewed
Sep 24, 2020
erwinmombay
force-pushed
the
fix-posthtml-transformers
branch
from
d3751c2
to
65a1295
Compare
|
@caroqliu adding you as i would love to get typescript feedback! |
erwinmombay
force-pushed
the
fix-posthtml-transformers
branch
from
65a1295
to
3a4f275
Compare
caroqliu
reviewed
Sep 28, 2020
build-system/server/new-server/transforms/utilities/option-set.ts
Outdated
Show resolved
Hide resolved
kristoferbaxter
approved these changes
Sep 28, 2020
erwinmombay
force-pushed
the
fix-posthtml-transformers
branch
2 times, most recently
from
ae9fc35
to
e87eee7
Compare
erwinmombay
force-pushed
the
fix-posthtml-transformers
branch
5 times, most recently
from
b7d113f
to
bc02e4d
Compare
Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
erwinmombay
force-pushed
the
fix-posthtml-transformers
branch
from
76bc7ae
to
74e4680
Compare
rcebulko
approved these changes
Oct 14, 2020
rsimha
reviewed
Oct 14, 2020
Comment on lines
+26
to
+28
| return `node .\\node_modules\\typescript\\lib\\tsc.js -p ${SERVER_TRANSFORM_PATH.split( | ||
| '/' | ||
| ).join(pathModule.sep)}${pathModule.sep}tsconfig.json`; |
There was a problem hiding this comment.
I think it might be possible to replace this split / join block with a path.posix method.
/cc @samouri, who has experience with this, and the vagaries of Windows.
| if (result.status != 0) { | ||
| const err = new Error('Could not build AMP Dev Server'); | ||
| err.showStack = false; | ||
| err.showStack = true; |
There was a problem hiding this comment.
The reason this was false in the first place was because the message in err contained the useful info, while the stack contained a very verbose and mostly useless nodejs callstack. Has any of this changed? (If yes, you can delete this line, since the default is true. If not, I'd suggest reverting this line.)
ed-bird
pushed a commit
to ed-bird/amphtml
that referenced
this pull request
Dec 10, 2020
…mpproject#30292) * 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 * fix imports * 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 <10456171+caroqliu@users.noreply.github.com> * Update build-system/server/new-server/transforms/utilities/option-set.ts Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * 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 * fix windows issues * skip failing IE test temporarily * applied recs * apply more recs * temporarily read from testing version.txt * temp "only" * dont reroute to max json files * change error to warning * fix max builds and integration tests against max files * revert testing changes. we'll separete it into its own PR Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
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.
To make gulp integration work cleanly with mjs files we transform the fixtures/* html files to the module script during
gulp integration --esm.fixes:
type="application/json"feature adds:
Related PR(s):
#30378 (add mjs server routes)
#28336 (turn on esm tests)