🐛 building v0-module.js compatible for script module #16045
Conversation
gulpfile.js
Outdated
| @@ -894,6 +896,30 @@ function dist() { | |||
| if (argv.fortesting) { | |||
| return enableLocalTesting(minifiedRuntimeTarget); | |||
| } | |||
| }).then(() => { | |||
| /* Copy v0.js to v0-nomodule.js and fix and | |||
There was a problem hiding this comment.
If this functionality is moved to it's own discreet file it might be easier to invoke the code with a function.
Also, this explanation can be expanded in a separate file. For instance... why does a <script type=module require no top level references to this? The explanation will help other developers (and yourself!) if this code needs to be modified later.
There was a problem hiding this comment.
Moved to a different file
gulpfile.js
Outdated
| return gulp.src(minifiedRuntimeTarget) | ||
| .pipe(gulpRename('v0-module.js')) | ||
| .pipe(gulpReplace(/global\?global:[a-z]*}\(this\)/, function(string) { | ||
| return `global?global:${string.match(/global\?global\:([a-z]*)\}\(this\)/)[1]}}(self)`; |
There was a problem hiding this comment.
Extract this gulpReplace into a single helper function.
There was a problem hiding this comment.
Moved to a single file. Do you mean a separate function just for replace plugin?
gulpfile.js
Outdated
| return gulp.src('dist/amp4ads-v0.js') | ||
| .pipe(gulpRename('dist/amp4ads-v0-module.js')) | ||
| .pipe(gulpReplace(/global\?global:[a-z]*}\(this\)/, function(string) { | ||
| return `global?global:${string.match(/global\?global\:([a-z]*)\}\(this\)/)[1]}}(self)`; |
There was a problem hiding this comment.
Extract this gulpReplace into a single helper function.
| }).then(() => { | ||
| return createModuleCompatibleES5Bundle('v0.js'); | ||
| }).then(() => { | ||
| return createModuleCompatibleES5Bundle('amp4ads-v0.js'); |
There was a problem hiding this comment.
What about shadow doc and web worker entrypoints?
There was a problem hiding this comment.
we are just replacing these two files in phase1.
rest remain as in.
| exports.createModuleCompatibleES5Bundle = function(src) { | ||
| return gulp.src('dist/' + src) | ||
| .pipe(gulpRename(src.substring(0, src.indexOf('.js')) + '-module.js')) | ||
| .pipe(gulpReplace(/global\?global:[a-z]*}\(this\)/, function(string) { |
There was a problem hiding this comment.
Can you give a sample of what will be replaced?
| */ | ||
| exports.createModuleCompatibleES5Bundle = function(src) { | ||
| return gulp.src('dist/' + src) | ||
| .pipe(gulpRename(src.substring(0, src.indexOf('.js')) + '-module.js')) |
There was a problem hiding this comment.
Nit: src.replace(/\.js$/, '-module.js')
| exports.createModuleCompatibleES5Bundle = function(src) { | ||
| return gulp.src('dist/' + src) | ||
| .pipe(gulpRename(src.replace(/\.js$/, '-module.js'))) | ||
| .pipe(gulpReplace(/global\?global:[a-z]*}\(this\)/, function(string) { |
There was a problem hiding this comment.
This can be rewritten: gulpReplace(/global\?global:(\w*)}\(this\)/, 'global?global:$1}(self)')
There was a problem hiding this comment.
We can get really picky and `/(global?global:\w*})(this)/, '$1(self)'. 😉
| .pipe(gulpReplace(/global\?global:[a-z]*}\(this\)/, function(string) { | ||
| return `global?global:${string.match(/global\?global\:([a-z]*)\}\(this\)/)[1]}}(self)`; | ||
| })) | ||
| .pipe(gulpReplace(/(global\?global:\w*})(this)/, '$1(self)')) |
There was a problem hiding this comment.
Missing an escape around the parenthesis.
There was a problem hiding this comment.
Thanks for catching that 👍
this->selfin the snippet inserted by closure compiler fromhttps://github.com/google/closure-compiler/blob/36f332788d54803c3c1afe06a9d84bf4b9f4945b/src/com/google/javascript/jscomp/js/util/global.js#L44