Strip additional_paths from the asset paths generated in the file.js rule #403

paypro-leon · 2024-01-09T16:53:03Z

Summary

#283 caused the top dir to be no longer stripped in the file.js rule. This exposes too much of the paths when additional_paths are used for assets within these paths.

For example when setting additional_paths like app/assets and with an asset at app/assets/images/image.svg would get a path like this static/app/assets/images/image.svg

In this PR the path will be static/images/image.svg

Pull Request checklist

Add/update test to cover these changes
Update documentation
Update CHANGELOG file

justin808 · 2024-01-10T04:32:27Z

Can you please add:

Changes to README or other docs
Tests

tomdracz

Great addition. LGTM

paypro-leon · 2024-01-10T09:40:09Z

@justin808 I didn't find any documentation about stripping the paths in the file.js rule. Do we want to add this somewhere in the README?

I added a test case to package/rules/tests/file.js or would you like to add more?

justin808 · 2024-01-15T08:02:23Z

@justin808 I didn't find any documentation about stripping the paths in the file.js rule. Do we want to add this somewhere in the README?

I added a test case to package/rules/tests/file.js or would you like to add more?

Thanks for the tests!

Yes, a few docs would be AWESOME. Thanks!

paypro-leon · 2024-01-19T15:41:56Z

@justin808 I have added a mention of this change in the README

justin808 · 2024-01-20T02:23:35Z

Is this a breaking change?

Let's get some people to try this out and confirm it's not breaking:
https://rubygems.org/gems/shakapacker/versions/7.3.0.beta.1

Otherwise, I need to bump the major version.

@G-Rath @tomdracz @ahangarha @Judahmeek

G-Rath · 2024-01-20T21:23:26Z

If I'm understanding this correctly, I don't think the introduction of includePaths is required and while it might be useful that change looks to be the result of a lot of the diff - I think it would be best if you reverted that for now, and open a dedicated issue if you want to make a case for it.

As for if this is breaking, I think that depends on the intended purpose of additional_paths: I've not ever used it and so far my attempts at trying to use it are giving errors. If the intent is to effectively include additional assets as if they were directly compiled by Shakapacker then I'd say this is probably a bug especially if these paths are mostly automated (i.e. apps should not require any changes after updating to continue working as normal, even if maybe a few caches are busted due to a slight url change)

But lets get the includePaths stuff reverted first and go from there

ahangarha · 2024-01-21T13:44:50Z

I agree with @G-Rath regarding changing our configuration and replacing some entries with includePaths. I don't think the benefit of this change justifies introducing such a breaking change in the project. It is also not contributing to this particular PR. It is a separate thing and can be discussed and possibly implemented in a separate issue and PR.

Regarding the key change of this PR, I don't think it would break anything except in a project that has a separate part that depends on a very specific asset URL.

To be in a safe margin, I propose we make this feature optional for version 7 and mention in our documentation that this behavior will be permanent in our next major release. And to activate this feature, we may choose one the following:

a new temporary configuration in config/shakapacker.yml that will be removed in version 8.
an env variable like SHAKAPACKER_STRIP_ADDITIONAL_PATH.

We can keep it experimental in version 7 and default in version 8.

paypro-leon · 2024-01-22T10:45:10Z

@G-Rath @ahangarha I reverted the includePath changes and made the change opt-in by setting SHAKAPACKER_STRIP_ADDITIONAL_PATH. Adding it to config/shakapacker.yml would introduce a change to the Config interface just like with includePath so this feels like the least intrusive change.

G-Rath · 2024-01-22T17:22:06Z

package/rules/file.js

-      const folders = dirname(pathData.filename)
-        .replace(`${sourcePath}`, '')
+      const path = dirname(pathData.filename)
+      const stripPaths = process.env.SHAKAPACKER_STRIP_ADDITIONAL_PATHS === 'true' ? [...additionalPaths, sourcePath] : [sourcePath]


we might as well pull this from shakapacker.yml since we're accessing that already, and then it'll be easier to ensure its set consistency

While I agree it will be easier to use shakapacker.yml would that not result in more config? I think we should be careful adding more to shakapacker.yml file, especially if this is going to be temporary.

Maybe we should choose if this is going to be configurable option or just default behavior. Based on that we can decide what the best place for this should be. I leave the final decision to you guys and will change it accordingly.

The "more config" is already being added by introducing an env variable, it's just harder to use - pulling it from shakapacker.yml means applications that use this can ensure it's always enabled properly which is a lot harder to do with an env variable.

What you're referring to is actually a seperate but very good question - what is the long term plan for this? Personally I'd feel better discussing that without external metrics if I had a sample app that actually used additional paths so I can see the different behaviors in action.

I would say if no one currently has strong feels to why this shouldn't be the new default behaviour then we should tie this new config variable with a message that shows at least when its not set (and maybe if it's set to false?) saying "hey we're planning to make this the default in the next major unless anyone objects - you should opt in to it now by setting ; please open an issue if this doesn't work or breaks your app or ... etc".

While it might seem verbose, I think it's very reasonable in this kind of situation since ultimately we can't not make improvements and also can't know about the requirements of all the applications out there so it's to users to report back about their use cases 🤷

I think it is better to have minimum temporary changes in our config for an experimental feature. We should remove them in our next major release so better to keep it on the side, and just give a way for activating it.

But this is my thought. The decision is not technical (all options are possible) but a PM one.

@justin808 What do you think about how to incorporate this feature in this version and then in v8?

@justin808 @G-Rath @ahangarha @tomdracz Has any decision been made if this is going to be added? I personally don't mind either option. Lets choose one and I will update the PR.

package/rules/less.js

justin808 · 2024-03-23T00:40:16Z

@G-Rath should this PR be changed to be on the v8 branch?

G-Rath · 2024-03-23T00:47:44Z

@justin808 I think first more investigation is needed to decide what should happen - I personally don't have much experience with this part of Shakapacker and so far have not been able to produce any notable difference in behaviour when trying to change the general settings to begin with.

If someone were to create a sample app that enabled exploring behaviours with different values that would at least help me to form an opinion on what I think we should do here.

As it is now though I've not seen a lot of activity to indicate this is a pressing issue so I don't think it should delay the release of v8

tomdracz · 2024-03-23T10:32:54Z

@G-Rath Repro here: tomdracz/shakapacker-css-test@1f2e367

I've added additional paths directory with one svg file and another svg file in main directory.

import 'images/test_additional.svg'
require.context('../images', true);

in pack js makes it so the relevant entries are pulled into manifest. images/test_additional.svg is resolved as we're looking into additional_paths when resolving imports.

In manifest though, they end up like below:

  "static/app/assets/images/test_additional.svg": "/packs/static/app/assets/images/test_additional-7a96f819dc16a03ee8e8.svg",
  "static/images/test.svg": "/packs/static/images/test-7a96f819dc16a03ee8e8.svg",

That additional path not being stripped, means the SVG from additional directory needs to be references as:

<%= image_pack_tag('app/assets/images/test_additional.svg') %>

I think removing that app/assets from output is sensible here as that just indicates entry directory (but images or anything further below that in hierarchy should be retained). Was toying with idea of outputting them to another directory like static/additional but that feels even more magical.

Note that this only affects static files. I can have a JS referenced in additional paths and it will be simply spit out in public/packs/js directory.

This is less of a problem if your imports are only being referenced in JS files, moreso if they're referenced in the views IMO.

G-Rath · 2024-03-23T18:06:19Z

@tomdracz thanks for that, I agree we should be stripping out the additional paths prefix out of the box.

@paypro-leon do you have the bandwidth to rebase this onto next and remove the option? otherwise me or @tomdracz can pick it up

tomdracz · 2024-03-24T10:03:35Z

Since we're looking to lend this in v8, I would say let's just shop without option to opt-in/out and just mark as breaking change. The new handling is reasonable to be default and can't see compelling reason to specifically opt-out 🤔

paypro-leon · 2024-03-26T09:10:22Z

@G-Rath I can pick it up and will remove the option.

…ditional-paths-in-file-rule

paypro-leon · 2024-03-26T11:30:30Z

Not sure why the job fails. Can we rerun the action manually or do I need to create a new commit?

tomdracz · 2024-03-26T12:56:51Z

Rerun kicked in, LGTM. Thanks @paypro-leon !

…rule (#403) * Add config.includePaths and strip includePaths from file.js output paths * Update CHANGELOG.md * Refactor code to fix lint issue * Make sure to replace only once * Change variable name * Update README.md * Grammer change * Revert includePath change and make stripping additional_paths opt-in * Update CHANGELOG.md * Revert some more changes * Revert changes * Remove opt-in check * Update README and CHANGELOG

* Remove deprecated webpacker spelling (#429) * feat!: drop support for Ruby 2.6 (#415) * feat!: remove `https` option (#414) * feat!: remove `relative_url_root` option (#413) * feat!: drop support for Node v12 (#431) * feat!: remove `yarn_install` task (#412) * ci: cancel in-progress jobs when new changes are pushed (#432) * ci: merge ruby and node based jobs into single workflows (#434) * ci: merge node related checks into a single workflow * ci: merge ruby related checks into a single workflow * ci: rename jobs * ci: test against Ruby 3.1 and 3.2, and Node 20 (#433) * ci: test against Ruby 3.1 and 3.2 * ci: test against Node 20 * ci: only run Rubocop with latest Ruby (#438) * chore: update pull request template (#437) * ci: test against Rails 7.1 (#436) * feat!: remove `globalMutableWebpackConfig` (#439) * ci: don't run on pushes to `next` (#444) * feat!: remove `verify_file_existance` method (#446) * feat!: remove `check_yarn` task (#443) * ci: use latest bundler in generator jobs (#445) * feat!: enable `ensure_consistent_versioning` by default (#447) * Remove dependency on glob (#435) * Remove dependency on glob * Pass in shakapacker config consistently (#448) * Reference config from config rather than instance in compiler * Strip additional_paths from the asset paths generated in the file.js rule (#403) * Add config.includePaths and strip includePaths from file.js output paths * Refactor code to fix lint issue * Make sure to replace only once * Change variable name * Revert includePath change and make stripping additional_paths opt-in * refactor: use snake_case for method name (#450) * fix: only strip paths that start with source path (#451) * feat!: always use `package_json` for managing node packages (#430) * feat!: minor cleanup of js utilities (#454) * test: require `ostruct` explicitly (#460) * ci: only run ESLint on latest Node (#462) * docs: create upgrade guide for v8 (#458) * V8 upgrade guide * docs: write v8 upgrade doc * docs: format with Prettier * feat!: move tests and helpers into (#459) * test: move into `test` directory * test: update import paths * test: update jest config * refactor: move test helpers * test: move jest config into dedicated file * docs: add changelog entry * chore: remove unneeded eslint ignore * refactor: update js linting (#461) * refactor: setup and apply Prettier (#463) * refactor: use `eslint-plugin-jest` (#464)

paypro-leon added 5 commits January 9, 2024 17:19

Add config.includePaths and strip includePaths from file.js output paths

2302cc4

Update CHANGELOG.md

70d9dc0

Refactor code to fix lint issue

70ca392

Make sure to replace only once

f907070

Change variable name

3364cd5

tomdracz approved these changes Jan 10, 2024

View reviewed changes

Update README.md

8307858

Grammer change

953c2d2

paypro-leon added 2 commits January 22, 2024 11:24

Revert includePath change and make stripping additional_paths opt-in

f369e21

Update CHANGELOG.md

a3493c3

Revert some more changes

bd2dd2e

G-Rath suggested changes Jan 22, 2024

View reviewed changes

Revert changes

5953225

G-Rath mentioned this pull request Mar 15, 2024

pending v8 changes #410

Merged

3 tasks

paypro-leon changed the base branch from main to next March 26, 2024 09:43

Remove opt-in check

8fd4ae6

paypro-leon added 2 commits March 26, 2024 10:56

Merge branch 'next' of github.com:shakacode/shakapacker into strip-ad…

d2d09b7

…ditional-paths-in-file-rule

Update README and CHANGELOG

d726a7b

paypro-leon requested review from G-Rath, tomdracz and ahangarha March 26, 2024 10:14

tomdracz approved these changes Mar 26, 2024

View reviewed changes

tomdracz merged commit 9bbce9d into shakacode:next Mar 26, 2024
52 checks passed

G-Rath mentioned this pull request Mar 26, 2024

fix: only strip paths that start with source path #451

Merged

3 tasks

coderabbitai bot mentioned this pull request May 8, 2024

Update CHANGELOG.md #469

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strip additional_paths from the asset paths generated in the file.js rule #403

Strip additional_paths from the asset paths generated in the file.js rule #403

paypro-leon commented Jan 9, 2024 •

edited

Loading

justin808 commented Jan 10, 2024

tomdracz left a comment

paypro-leon commented Jan 10, 2024

justin808 commented Jan 15, 2024

paypro-leon commented Jan 19, 2024

justin808 commented Jan 20, 2024

G-Rath commented Jan 20, 2024

ahangarha commented Jan 21, 2024

paypro-leon commented Jan 22, 2024

G-Rath Jan 22, 2024

paypro-leon Jan 23, 2024

G-Rath Jan 26, 2024

ahangarha Jan 27, 2024

paypro-leon Mar 14, 2024

justin808 commented Mar 23, 2024

G-Rath commented Mar 23, 2024

tomdracz commented Mar 23, 2024 •

edited

Loading

G-Rath commented Mar 23, 2024

tomdracz commented Mar 24, 2024

paypro-leon commented Mar 26, 2024

paypro-leon commented Mar 26, 2024 •

edited

Loading

tomdracz commented Mar 26, 2024

Strip additional_paths from the asset paths generated in the file.js rule #403

Strip additional_paths from the asset paths generated in the file.js rule #403

Conversation

paypro-leon commented Jan 9, 2024 • edited Loading

Summary

Pull Request checklist

justin808 commented Jan 10, 2024

tomdracz left a comment

Choose a reason for hiding this comment

paypro-leon commented Jan 10, 2024

justin808 commented Jan 15, 2024

paypro-leon commented Jan 19, 2024

justin808 commented Jan 20, 2024

G-Rath commented Jan 20, 2024

ahangarha commented Jan 21, 2024

paypro-leon commented Jan 22, 2024

G-Rath Jan 22, 2024

Choose a reason for hiding this comment

paypro-leon Jan 23, 2024

Choose a reason for hiding this comment

G-Rath Jan 26, 2024

Choose a reason for hiding this comment

ahangarha Jan 27, 2024

Choose a reason for hiding this comment

paypro-leon Mar 14, 2024

Choose a reason for hiding this comment

justin808 commented Mar 23, 2024

G-Rath commented Mar 23, 2024

tomdracz commented Mar 23, 2024 • edited Loading

G-Rath commented Mar 23, 2024

tomdracz commented Mar 24, 2024

paypro-leon commented Mar 26, 2024

paypro-leon commented Mar 26, 2024 • edited Loading

tomdracz commented Mar 26, 2024

paypro-leon commented Jan 9, 2024 •

edited

Loading

tomdracz commented Mar 23, 2024 •

edited

Loading

paypro-leon commented Mar 26, 2024 •

edited

Loading