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

Removed the useRelativePaths step #25

Merged
merged 3 commits into from
Mar 10, 2023
Merged

Conversation

ijlee2
Copy link
Owner

@ijlee2 ijlee2 commented Mar 10, 2023

Background

When migrating a v1 addon to v2, whether absolute or relative paths are used doesn't affect the build and is a question of code style.

In addition to preserving the end-developer's code better, removing useRelativePaths has another benefit. The current implementation, which relies on a regular expression, is hard to maintain and can result in false positives (a bug). These problems were observed in:

Bug report from ember-container-query

Bug fix for useRelativePaths

Feedback from @phndiaye

While replacing old imports (with the package’s name as root) with relative imports, the codemod accidentally replaced dynamically built asset paths in some components with relative imports, breaking some images paths. A quick double check might be needed here

For the translations, although there is a default directory, it can be configured to have a custom directory so it would be some extra work for the codemod to read the ember-intl configuration and match it.

For the false positive in the relative paths, I got something like this in component:

class extends Component {
  basePath: string = '@upfluence/hyperstream/assets/images';

  get illustration(): string {
    if (this.args.stream.count === 0) {
      return `${this.basePath}/empty-${this.args.stream.active ? 'active' : 'paused'}-stream.svg`;
    }

    return `${this.basePath}/no-matches.svg`;
  }
}

which was then used in the template like <img src={{asset-map this.illustration}} />. [T]he @upfluence/hyperstream in the basePath was rewritten to ../../../assets/images.

Feedback from @josephdsumner

I think I would echo the feedback that came up in the blog post / thread in community chat -- the relative paths being perhaps optional, how things like less / postcss might get updated.

The underlying issue is, a regular expression doesn't understand files to be able to distinguish false positives from true ones. A more powerful library would be needed to parse CSS / JS / TS / ... files, but bringing in such a library won't provide much value (again, when we consider relative paths as a stylistic choice).

What changed?

For simplicity, I removed the useRelativePaths step and the associated test files. The removal has a few benefits:

  • There's less code to maintain.
  • The end-developer's code is preserved better.
  • The end-developer can have an easier time getting the scripts for the addon and test-app packages to run.

@ijlee2 ijlee2 added the bug Something isn't working label Mar 10, 2023
@ijlee2 ijlee2 marked this pull request as ready for review March 10, 2023 08:46
@ijlee2 ijlee2 merged commit 31b2678 into main Mar 10, 2023
@ijlee2 ijlee2 deleted the bugfix-remove-use-relative-paths branch March 10, 2023 08:46
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.

1 participant