Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

fix: adding .js extension to exported *.css.js file #4364

Conversation

mathisscott
Copy link
Contributor

• this was throwing off some versions/configurations of Jest
• the module loader was looking for a CSS file

Signed-off-by: Scott Mathis smathis@vmware.com

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • clarity.design website / infrastructure changes
  • Other... Please describe:

What is the current behavior?

We have to export the base element styles. These are generated *.css.js styles.

Some configs/implementations of Jest don't try to look for *css.js. They stop looking at *.css.

Issue Number: N/A

What is the new behavior?

I added the full path. This is the only Clarity issue I've been able to find that impedes Jest configuration. Every other issue in #4094 and #4196 are (thus far) solvable by properly configuring an application.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

• this was throwing off some versions/configurations of Jest
• the module loader was looking for a CSS file

Signed-off-by: Scott Mathis <smathis@vmware.com>
@mathisscott
Copy link
Contributor Author

I have no idea if we want to leave #4094 and #4196 open after this is merged.

No other issues in those tickets are caused by or related to Clarity.

@mathisscott mathisscott self-assigned this Mar 12, 2020
@@ -24,4 +24,4 @@ export * from './mixins/css-helpers';
export * from './mixins/unique-id';
export * from './mixins/apply-mixins';
export * from './interfaces';
export { styles as baseStyles } from './base/base.element.css';
export { styles as baseStyles } from './base/base.element.css.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fix the issue temporarily for Jest but once we pull Core components into clr-angular it will break again. We should update all the relative import paths in core to have .js at the end like this one. This is a fundamental issue with TypeScripts output not including the .js file extension. See discussion here microsoft/TypeScript#16577

You can see here lit-html and lit-element do the same thing to get around this issue.
https://github.com/Polymer/lit-element/blob/master/src/lit-element.ts#L18

This would make this a pretty large PR so we can merge this to fix the Jest issue now and I can open a new issue describing the steps to address the rest or we tackle this now in this PR. Either way is fine.

Copy link
Contributor

@gnomeontherun gnomeontherun left a comment

Choose a reason for hiding this comment

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

I approve but request a backlog ticket to be opened for https://github.com/vmware/clarity/pull/4364/files#r391847610

@mathisscott mathisscott merged commit d21a375 into vmware-archive:master Mar 12, 2020
@mathisscott
Copy link
Contributor Author

@gnomeontherun
See #4365

@mathisscott mathisscott added @cds/core resolution: fixed type: build Includes issues that pertain to webpack, docker, travis, gemini, and the kitchen sink app type: support Support, implementation or questions labels Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@cds/core resolution: fixed type: build Includes issues that pertain to webpack, docker, travis, gemini, and the kitchen sink app type: support Support, implementation or questions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants