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

Support for classic apps #33

Merged
merged 16 commits into from
May 25, 2023
Merged

Support for classic apps #33

merged 16 commits into from
May 25, 2023

Conversation

candunaj
Copy link
Contributor

This PR adds support for classic apps.
CSS files are rewritten by css preprocessor.
A babel plugin rewrites templates.

@candunaj candunaj added the enhancement New feature or request label May 24, 2023
@candunaj candunaj marked this pull request as ready for review May 25, 2023 09:04
Copy link
Contributor

@mansona mansona left a comment

Choose a reason for hiding this comment

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

All together this is very very good 🎉 I had a few comments but nothing too bad

Comment on lines +22 to +24
let htmlbarsPlugin = plugins.find(
(p) => p._parallelBabel?.params?.templateCompilerPath
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we think this is good enough to identify the htmlbars plugin? there are no other more specific ways to find this?


let customPlugin = [require.resolve('../dist/scoped-babel-plugin.cjs')];

plugins.splice(htmlbarsPluginIndex - 1, 0, customPlugin);
Copy link
Contributor

Choose a reason for hiding this comment

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

does this break if htmlbarsPlugin is the first in the list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

let htmlbarsPlugin = plugins.find(
(p) => p._parallelBabel?.params?.templateCompilerPath
);
if (htmlbarsPlugin) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not need an else with this if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how it could happen. I believe, that the plugin has to be there in every application.

Comment on lines +41 to +46
let preprocessors = registry.load('css');
preprocessors.forEach((p) => registry.remove('css', p));
this.outputStylePreprocessor.preprocessors = preprocessors;
registry.add('css', this.outputStylePreprocessor);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment ahead of this to explain that we needed to make sure that we run ours first (and that we couldn't find any ember-cli way to make it happen first)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for suggestion

Comment on lines +29 to +32
// ignore css files for ember-css-modules
if (relativePath.endsWith('.module.css')) {
return '';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this needs to be configurable 🤔 since ember-css-modules could be configured with anything

@candunaj candunaj merged commit 5923a66 into main May 25, 2023
@candunaj candunaj deleted the classic-app branch May 25, 2023 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants