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

Force the Javascript target definition #86

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

ckrina
Copy link
Contributor

@ckrina ckrina commented Mar 15, 2022

Problem

While trying to define the JS assets in a new theme I found out that setting the variable DRAINPIPE_JAVASCRIPT in Taskfile.yml wasn't working properly: whatever I set in there it'd export it in another directory right below docroot.

I set:

DRAINPIPE_JAVASCRIPT: |
    docroot/themes/custom/my_theme/src/script.js:docroot/themes/custom/my_theme/src/script.min.js

And instead of exporting it into docroot/themes/custom/my_theme/src/script.min.js, it was creating the files into docroot, ignoring the target path.

Proposed solution

@hawkeyetwolf helped me coming up with this possible solution, forcing the definition of the outbase value, and now the source path set is being used for the target too.

But while reading the comments about the limitation to provide separate entryNames, wouldn't it be less confusing to just provide an entry value for any JS on the Taskfile.yml and use that to build both for the source and the value path?

@justafish what do you think? Does the changes makes sense? Is it enough, or should we try to improve something else?

@ckrina ckrina requested a review from justafish March 15, 2022 17:42
@justafish
Copy link
Member

@ckrina Would it be possible to setup an example barebones repo reproducing this? We have the following setup on another project and it seems to be working fine:

  DRAINPIPE_JAVASCRIPT: |
    web/themes/custom/bookworm/script.js:web/themes/custom/bookworm/script.min.js
    web/themes/custom/ghostwriter/script.js:web/themes/custom/ghostwriter/script.min.js
    web/modules/custom/[client]_design/script.js:web/modules/custom/[client]_design/script.min.js

@hawkeyetwolf
Copy link
Contributor

hawkeyetwolf commented Apr 4, 2022

@justafish thank you taking a look. Yes to being able to reproduce on a barebones repo. The example you gave works because of two things:

  • the entries use two different directories just below root (web/themes and web/custom), which causes the outbase variable to be set to web automatically.
  • All the output directories match the source directories.

If either of those things were changed, the JS would compile to unexpected directories. Will you try changing either of those in your project and let me know if you see the same? This behavior can also be confirmed by looking at the code to see that the destination directories are not being passed to esbuild.

@justafish
Copy link
Member

Thanks!

@justafish justafish merged commit 8af1775 into Lullabot:main Apr 12, 2022
@hawkeyetwolf
Copy link
Contributor

FYI this still doesn't fix the issue of ignoring the destination directories. Regardless of what destination directory is provided, the compiled JS will always land next to the source file. This is an okay limitation, but we should update the Drainpipe docs to entirely remove the mention of a destination directory. Is that how you'd like to proceed, @justafish, or would you rather we open a new issue for using the destination directories?

@ckrina ckrina deleted the ckrina/js-asset-export-path branch April 12, 2022 15:38
@justafish
Copy link
Member

@hawkeyetwolf let's open a new issue please 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants