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

[bugfix] Ensured that normalizeFileExt ignores .css.d.ts files #1450

Merged
merged 3 commits into from
May 25, 2023
Merged

[bugfix] Ensured that normalizeFileExt ignores .css.d.ts files #1450

merged 3 commits into from
May 25, 2023

Conversation

ijlee2
Copy link
Contributor

@ijlee2 ijlee2 commented May 25, 2023

Background

I created sample-v2-addon to investigate how we can introduce CSS modules to v2 addons (while supporting Glint). The key is, for each CSS file *.css, to create the corresponding declaration file *.css.d.ts.

sample-v2-addon
├── src
│   └── components
│       ├── navigation-menu.css
│       ├── navigation-menu.css.d.ts
│       ├── navigation-menu.hbs
│       └── navigation-menu.ts
...

When the addon's publicEntrypoints and appReexports include the "normal" wildcard pattern components/**/*.js, the resulting dist will include files that are not supposed to be present:

sample-v2-addon
├── dist
│   └── components
│       ├── navigation-menu.css.d.d.ts ❌
│       ├── navigation-menu.css.d.d.ts.map ❌
│       ├── navigation-menu.css.d.js ❌
│       ├── navigation-menu.css.d.js.map ❌
│       ├── navigation-menu.d.ts
│       ├── navigation-menu.js
│       └── navigation-menu.js.map
...

I worked around the problem by providing the pattern components/**/!(*.css.d).js, but find the additional complexity in regular expression to be a tech risk (not easy to understand—what is .css.d.js?—and maintain):

sample-v2-addon
├── dist
│   └── components
│       ├── navigation-menu.d.ts
│       ├── navigation-menu.d.ts.map
│       ├── navigation-menu.js
│       └── navigation-menu.js.map
...

Proposed solution

I suspect, the current implementation of normalizeFileExt didn't consider file extensions that include .ts as a substring.

function normalizeFileExt(fileName: string) {
  return fileName.replace(/\.ts|\.hbs|\.gts|\.gjs$/, '.js');
}

By making an early exit, we can consider *.ts cases and define a more specific rule that ignores .css.d.ts.

function normalizeFileExt(fileName: string) {
  if (fileName.endsWith('.ts')) {
    // Match .ts but not .d.ts
    const regex = /(^.?|\.[^d]|[^.]d|[^.][^d])\.ts$/;

    return fileName.replace(regex, '$1.js');
  }

  return fileName.replace(/\.hbs|\.gts|\.gjs$/, '.js');
}

Admittedly, the regular expression /(^.?|\.[^d]|[^.]d|[^.][^d])\.ts$/, copied from StackOverflow, is complex and forms another tech risk. Ideally, unit tests for normalizeFileExt would be present to document its input and output. (I didn't know in which package such tests can be written.)

@ijlee2 ijlee2 changed the title Ensured that normalizeFileExt doesn't change .css.d.ts files to .css.d.js [bugfix] Ensured that normalizeFileExt doesn't change .css.d.ts files to .css.d.js May 25, 2023
@ijlee2 ijlee2 marked this pull request as ready for review May 25, 2023 07:20
@ijlee2 ijlee2 changed the title [bugfix] Ensured that normalizeFileExt doesn't change .css.d.ts files to .css.d.js [bugfix] Ensured that normalizeFileExt ignores .css.d.ts files May 25, 2023
Copy link
Contributor

@ef4 ef4 left a comment

Choose a reason for hiding this comment

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

Thanks, this makes sense. Typescript caused a lot of headaches when they picked .d.ts to mean a different thing than .ts.

@@ -5,7 +5,14 @@ import minimatch from 'minimatch';
import type { Plugin } from 'rollup';

function normalizeFileExt(fileName: string) {
return fileName.replace(/\.ts|\.hbs|\.gts|\.gjs$/, '.js');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please check your use case and see if this implementation also solves it?

return fileName.replace(/(?<!\.d)\.ts|\.hbs|\.gts|\.gjs$/, '.js');

The idea here is "negative lookbehind assertions".

If this works, we can stick with only a single regex replace instead of nesting them.

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, I checked that your solution works in ijlee2/embroider-css-modules#46.

@ef4 ef4 merged commit da01d2b into embroider-build:main May 25, 2023
@ef4 ef4 added the bug Something isn't working label May 25, 2023
@ijlee2 ijlee2 deleted the bugfix-normalize-file-extension branch May 25, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants