Skip to content
This repository was archived by the owner on Jan 13, 2025. It is now read-only.

Commit 7c6da3a

Browse files
fix(infrastructure): Harden closure declaration source rewriting (#835)
- Ensure that all JS src files are recursively picked up when sending `--js` flag to closure compiler - Ensure `export ... from ...` statements are covered when rewriting declaration sources.
1 parent 184897b commit 7c6da3a

File tree

2 files changed

+22
-19
lines changed

2 files changed

+22
-19
lines changed

scripts/closure-test.sh

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ function log() {
2424

2525
CLOSURE_TMP=.closure-tmp
2626
CLOSURE_PKGDIR=$CLOSURE_TMP/packages
27-
JS_SRCS=$CLOSURE_PKGDIR/**/*.js
2827
CLOSURIZED_PKGS=$(node -e "console.log(require('./package.json').closureWhitelist.join(' '))")
2928

3029
if [ -z "$CLOSURIZED_PKGS" ]; then
@@ -42,7 +41,7 @@ done
4241
rm -fr $CLOSURE_PKGDIR/**/{node_modules,dist}
4342

4443
log "Rewriting all import statements to be closure compatible"
45-
node scripts/rewrite-import-statements-for-closure-test.js $CLOSURE_PKGDIR
44+
node scripts/rewrite-decl-statements-for-closure-test.js $CLOSURE_PKGDIR
4645

4746
log "Testing packages"
4847
echo ''
@@ -56,7 +55,7 @@ for pkg in $CLOSURIZED_PKGS; do
5655
CMD="java -jar node_modules/google-closure-compiler/compiler.jar \
5756
--externs closure_externs.js \
5857
--compilation_level ADVANCED \
59-
--js $JS_SRCS \
58+
--js $(find $CLOSURE_PKGDIR -type f -name "*.js") \
6059
--language_out ECMASCRIPT5_STRICT \
6160
--dependency_mode STRICT \
6261
--module_resolution LEGACY \

scripts/rewrite-import-statements-for-closure-test.js renamed to scripts/rewrite-decl-statements-for-closure-test.js

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
* - https://github.com/google/closure-compiler/issues/2386
5959
*
6060
* Note that for third-party modules, they must be defined in closure_externs.js. See that file for more info.
61+
* Also note that this works on `export .... from ...` as well.
6162
*/
6263

6364
const assert = require('assert');
@@ -94,8 +95,10 @@ function transform(srcFile, rootDir) {
9495
});
9596

9697
traverse(ast, {
97-
'ImportDeclaration'({node}) {
98-
rewriteImportDeclaration(node, srcFile, rootDir);
98+
'ImportDeclaration|ExportNamedDeclaration'({node}) {
99+
if (node.source) {
100+
rewriteDeclarationSource(node, srcFile, rootDir);
101+
}
99102
},
100103
});
101104

@@ -113,38 +116,39 @@ function transform(srcFile, rootDir) {
113116
console.log(`[rewrite] ${srcFile}`);
114117
}
115118

116-
function rewriteImportDeclaration(node, srcFile, rootDir) {
117-
let importSource = node.source.value;
118-
const pathParts = importSource.split('/');
119+
function rewriteDeclarationSource(node, srcFile, rootDir) {
120+
let source = node.source.value;
121+
const pathParts = source.split('/');
119122
const isMDCImport = pathParts[0] === '@material';
120123
if (isMDCImport) {
121124
const modName = pathParts[1]; // @material/<modName>
122125
const atMaterialReplacementPath = `${rootDir}/mdc-${modName}`;
123-
const rewrittenImportSource = [atMaterialReplacementPath].concat(pathParts.slice(2)).join('/');
124-
importSource = rewrittenImportSource;
126+
const rewrittenSource = [atMaterialReplacementPath].concat(pathParts.slice(2)).join('/');
127+
source = rewrittenSource;
125128
}
126129

127-
patchNodeForImportSource(importSource, srcFile, node);
130+
patchNodeForDeclarationSource(source, srcFile, rootDir, node);
128131
}
129132

130-
function patchNodeForImportSource(importSource, srcFile, node) {
131-
let resolvedImportSource = importSource;
133+
function patchNodeForDeclarationSource(source, srcFile, rootDir, node) {
134+
let resolvedSource = source;
132135
// See: https://nodejs.org/api/modules.html#modules_all_together (step 3)
133-
const wouldLoadAsFileOrDir = ['./', '/', '../'].some((s) => importSource.indexOf(s) === 0);
136+
const wouldLoadAsFileOrDir = ['./', '/', '../'].some((s) => source.indexOf(s) === 0);
134137
const isThirdPartyModule = !wouldLoadAsFileOrDir;
135138
if (isThirdPartyModule) {
136-
assert(importSource.indexOf('@material') < 0, '@material/* import sources should have already been rewritten');
139+
assert(source.indexOf('@material') < 0, '@material/* import sources should have already been rewritten');
137140
patchDefaultImportIfNeeded(node);
138-
resolvedImportSource = `goog:mdc.thirdparty.${camelCase(importSource)}`;
141+
resolvedSource = `goog:mdc.thirdparty.${camelCase(source)}`;
139142
} else {
140-
const needsClosureModuleRootResolution = path.isAbsolute(importSource);
143+
const normPath = path.normalize(path.dirname(srcFile), source);
144+
const needsClosureModuleRootResolution = path.isAbsolute(source) || fs.statSync(normPath).isDirectory();
141145
if (needsClosureModuleRootResolution) {
142-
resolvedImportSource = path.relative(rootDir, resolve.sync(importSource, {
146+
resolvedSource = path.relative(rootDir, resolve.sync(source, {
143147
basedir: path.dirname(srcFile),
144148
}));
145149
}
146150
}
147-
node.source = t.stringLiteral(resolvedImportSource);
151+
node.source = t.stringLiteral(resolvedSource);
148152
}
149153

150154
function patchDefaultImportIfNeeded(node) {

0 commit comments

Comments
 (0)