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

✨🏗 add cssBinaries options for extra css outputs for an extension #13303

Merged
merged 1 commit into from
Feb 8, 2018

Conversation

erwinmombay
Copy link
Member

@erwinmombay erwinmombay commented Feb 6, 2018

option will look like:

declareExtension('amp-user-notification', '0.1', {
  hasCss: true,
  cssBinaries: ['my-second-css']
});

Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

@newmuis Is amp-story still using the shadow DOM approach for style isolation?

gulpfile.js Outdated
if (options.compileOnlyCss) {
return Promise.resolve();
}
return buildExtensionJs(path, name, version, options);

Choose a reason for hiding this comment

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

IMO this should be extracted and called in buildExtension instead.

Copy link
Contributor

@rsimha rsimha Feb 6, 2018

Choose a reason for hiding this comment

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

We already have a function called buildExtension: https://github.com/ampproject/amphtml/blob/master/gulpfile.js#L508

Just re-read the above comment. Never mind :)

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

gulpfile.js Outdated
}
const promises = [];
const mainCssBinary = jsifyCssAsync(path + '/' + name + '.css')
.then(writeCssBinaries.bind(null, `${name}-${version}.css`))

Choose a reason for hiding this comment

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

Nit: I think this is a bit more readable.

.then(() => writeCssBinaries(`${name}-${version}.css`)) 

Copy link
Member Author

Choose a reason for hiding this comment

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

i would need to write it as .then(css => writeCssBinaries(`${name}-${version}.css`, css))
to get the equivalent. (I'm ok with rewriting it to this)

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@newmuis
Copy link
Contributor

newmuis commented Feb 6, 2018

@choumx We would like to use shadow DOM for isolation, but are blocked by this

* loadPriority: ?string,
* cssBinaries: ?Array<string>,
* extraGlobs?Array<string>,
* bundleOnlyIfListedInFiles: ?boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a declaration for the same key that's currently being used in buildExtensions, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

i think so. i was having a hard time tracking if its still the same options object but i believe so

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose testing with --files should make it clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it's worth testing this code with a couple variations of gulp | gulp dist --fortesting with the args --noextensions | --extensions=minimal_set since that behavior depends on CSS compilation.

Copy link
Member Author

Choose a reason for hiding this comment

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

done. tested

gulpfile.js Outdated
if (options.compileOnlyCss) {
return Promise.resolve();
}
return buildExtensionJs(path, name, version, options);
Copy link
Contributor

@rsimha rsimha Feb 6, 2018

Choose a reason for hiding this comment

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

We already have a function called buildExtension: https://github.com/ampproject/amphtml/blob/master/gulpfile.js#L508

Just re-read the above comment. Never mind :)

@newmuis
Copy link
Contributor

newmuis commented Feb 6, 2018

/cc @gmajoulet

@rsimha
Copy link
Contributor

rsimha commented Feb 6, 2018

FYI, @zhouyx is making some changes to how the CSS in extensions are built during gulp dist --noextensions in #13285, so the changes in the two PRs will likely need to be reconciled.

* loadPriority: ?string,
* cssBinaries: ?Array<string>,
* extraGlobs?Array<string>,
* bundleOnlyIfListedInFiles: ?boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose testing with --files should make it clear.

@erwinmombay erwinmombay force-pushed the multi-css-out branch 2 times, most recently from d33b915 to 489a3cc Compare February 7, 2018 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants