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

Loading uikit resources via the Node resolver #1246

Merged
merged 7 commits into from
Aug 26, 2020

Conversation

ringods
Copy link
Contributor

@ringods ringods commented Aug 26, 2020

This is a retry of #1225 which got reverted because of last-minute issues.

Summary of changes:

  • Resolving resources (css/templates) from uikits is done via the NodeJS resolver (require.resolve())

@ringods ringods requested a review from raphaelokon as a code owner August 26, 2020 14:49
@ringods ringods marked this pull request as draft August 26, 2020 14:49
@ringods ringods removed the request for review from raphaelokon August 26, 2020 14:49
@coveralls
Copy link

coveralls commented Aug 26, 2020

Coverage Status

Coverage increased (+2.3%) to 76.696% when pulling 2adb96d on feature/uikit-node-resolver into f4d6bd1 on dev.

@ringods
Copy link
Contributor Author

ringods commented Aug 26, 2020

https://deploy-preview-1246--patternlab-handlebars-preview.netlify.app/?p=all

@sghoweri @JosefBredereck can you review again? The tests succeed and the preview works now.

@ringods ringods requested a review from sghoweri August 26, 2020 15:44
@ringods ringods marked this pull request as ready for review August 26, 2020 15:44
@JosefBredereck JosefBredereck requested review from JosefBredereck and removed request for sghoweri August 26, 2020 16:50
Comment on lines +42 to +48
} else {
// For backwards compatibility, name to package calculation is:
// 1. name -> name
// 2. name -> uikit-name
// 3. name -> @pattern-lab/name
// 4. name -> @pattern-lab/uikit-name
for (const packageName of [
Copy link
Contributor

Choose a reason for hiding this comment

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

Good fallback

Comment on lines +70 to +74
} else {
logger.warning(
`Please update the configuration of UIKit ${uikitConfig.name} with property 'package: ${uikitConfig.package}' in patternlab-config.json. Lookup by 'name' is deprecated and will be removed in the future.`
);
} // [3]
Copy link
Contributor

Choose a reason for hiding this comment

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

Very good message as info for the user

Comment on lines +9 to +11
const resolveFileInPackage = (packageName, ...pathElements) => {
return require.resolve(path.join(packageName, ...pathElements));
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I checked the spread syntax. It is available since Node V8.x

Copy link
Contributor

@JosefBredereck JosefBredereck left a comment

Choose a reason for hiding this comment

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

All in all clean and good changes (☞゚ヮ゚)☞

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.

3 participants