Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep AMP_CONFIG writing in sync with production release script. #24471

Merged
merged 4 commits into from Sep 12, 2019

Conversation

lannka
Copy link
Contributor

@lannka lannka commented Sep 11, 2019

No description provided.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments below.

'amp-inabox.js',
'alp.max.js',
'integration.js',
'amp-shadow.js',
'amp-esm.js',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Alphabetize?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorted MINIFIED list in alphabetic order. and kept same ordering in this UNMINIFIED list

'amp4ads-v0.js',
'alp.js',
'f.js',
'shadow-v0.js',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no minified esm target?

/cc @erwinmombay @jridgewell

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see amp-esm.js referenced anywhere else. is it still a thing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is.

if (argv.esm) {
await Promise.all([
createModuleCompatibleES5Bundle('v0.js'),
createModuleCompatibleES5Bundle('amp4ads-v0.js'),
createModuleCompatibleES5Bundle('shadow-v0.js'),
]);
}

amphtml/babel.config.js

Lines 47 to 51 in b6d45be

// `dist` builds do not use any of the default settings below until its
// an esm build. (Both Multipass and Singlepass)
if (isDist && !esm) {
return {};
}

// Add babel plugin to remove unwanted polyfills in esm build
if (options.esmPassCompilation) {
compilationOptions['dest'] = './dist/esm/';
define.push('ESM_BUILD=true');
}

doBuildJs(jsBundles, 'amp.js', {
watch,
minify,
wrapper: wrappers.mainBinary,
singlePassCompilation: argv.single_pass,
esmPassCompilation: argv.esm,
includeOnlyESMLevelPolyfills: argv.esm,
}),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so is there still an output file named amp-esm.js ? seems it will just be amp.js?

Copy link
Contributor

@rsimha rsimha Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked, you are right. Sorry. Carry on :)

PS: We should probably remove amp-esm.js from UNMINIFIED_TARGETS.

'amp-inabox.js',
'alp.max.js',
'integration.js',
'amp-shadow.js',
'amp-esm.js',
];

/**
* List of minified targets to which AMP_CONFIG should be written
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might make sense to add a comment here to keep both lists in sync with the release scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@lannka lannka merged commit c9f40cf into ampproject:master Sep 12, 2019
@lannka lannka deleted the AMP_CONFIG_files branch September 12, 2019 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants