Skip to content

handling some gaps in compilation #1

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.

Sign up for GitHub

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

Open
reubenson wants to merge 2 commits into
base: main
Choose a base branch
from
Open

handling some gaps in compilation #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b6032f
handling some gaps in compilation
reubenson Sep 8, 2025
1fe96e0
additional fixes
reubenson Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/media/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,22 @@ import esbuild from 'esbuild';
import compiler from '../constants.js'

const outdir = path.join(process.cwd(), 'public', 'media');
const mediaGlobs = '*.+(jpg|jpeg|png|gif|webp|svg|ico)';
const mediaGlobs = '*.+(jpg|jpeg|png|gif|webp|svg|ico|js)';
const modules = {
components: {
path: path.join(process.cwd(), 'components'),
src: globby(path.join(process.cwd(), 'components', '**', 'media', mediaGlobs)),
},
layouts: {
path: path.join(process.cwd(), 'layouts'),
src: globby(path.join(process.cwd(), 'layouts', '**', 'media', mediaGlobs)),
},
Copy link
Author

@reubenson reubenson Sep 9, 2025

Choose a reason for hiding this comment

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

I wasn't seeing layout assets get processed, but maybe I'm missing some other flag to toggle that on?

Copy link
Member

@ealcantara22 ealcantara22 Sep 9, 2025

Choose a reason for hiding this comment

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

good catch. I missed that, and claycli definitely handles layout media files

sites: {
path: path.join(process.cwd(), 'sites'),
src: globby([
path.join(process.cwd(), 'sites', '**', 'media', mediaGlobs),
]),
}
},
}

/**
Expand Down Expand Up @@ -82,6 +86,7 @@ const mediaConfig = {
'.png': 'copy',
'.svg': 'copy',
'.webp': 'copy',
'.js': 'copy',
Copy link
Author

@reubenson reubenson Sep 9, 2025

Choose a reason for hiding this comment

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

slate codebase includes some .js assets in the media directory that could get handled this way

Copy link
Member

@ealcantara22 ealcantara22 Sep 9, 2025

Choose a reason for hiding this comment

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

Interesting, I'm wondering how those files are getting picked up and bundled by claycli as the .js extension is not listed here. Do these JS files in component's media dir require post-processing, or is simply copying them enough?

Copy link
Author

@reubenson reubenson Sep 9, 2025

Choose a reason for hiding this comment

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

oh good point. i see now that we're handling the copy-over of .js files in /media in a separate gulpfile, and not relying on clay cli. in our use case, these assets are meant to be self-contained scripts to be served as-is without need for post-processing. handling the js use case would be nice for keeping that sort of thing out of the component template file (i think)

Copy link
Member

@ealcantara22 ealcantara22 Sep 11, 2025

Choose a reason for hiding this comment

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

Yes, I can see the value in that. Please let me know when the PR is ready for review! 🙂

Copy link
Author

@reubenson reubenson Sep 11, 2025

Choose a reason for hiding this comment

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

thanks! i haven't had a chance yet to dig much into the edit mode issue - is that something you were seeing at all in local dev/testing?

Copy link
Member

@ealcantara22 ealcantara22 Sep 11, 2025

Choose a reason for hiding this comment

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

no! edit mode works on our end, even in watch mode. Can you please create an issue with steps on how to reproduce?

Copy link
Author

@reubenson reubenson Sep 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

looking into this more closely will create an issue

Copy link
Author

@reubenson reubenson Sep 15, 2025

Choose a reason for hiding this comment

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

Wrote up an issue here, but let me know if you're unable to reproduce the issue
#2

},
logLevel: compiler.logLever,
}
Expand Down
12 changes: 9 additions & 3 deletions src/scripts/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,13 +268,11 @@ async function processModule(filePath, cachedIds, registry, ids, envs) {
let prependContent = `window.modules["${moduleId}"] = [function(require,module,exports){`;
let appendContent = '';

const fileName = `${moduleId}.js`;

// compile vue to JS before creating the magic string
if (isVue) {
try {
const vueSFC = await vueSfcHandler({
filename: fileName,
filename: filePath,
Copy link
Author

@reubenson reubenson Sep 18, 2025

Choose a reason for hiding this comment

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

was getting an error processing assets in vue files like here, which filePath resolves. but maybe then breaks your use case?

<style lang="scss">
@use "../../../global/scss/utility/_visibility.scss" as visibility;

Copy link
Member

@ealcantara22 ealcantara22 Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, this is a new one. We're using postcss which is a different step so, maybe that's why we're not affected. Anyway, are you saying that replacing that line fixed the issue for you?

Copy link
Author

@reubenson reubenson Sep 24, 2025

Choose a reason for hiding this comment

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

Yup, making this change fixed the issue for Slate!

source: code,
extractCss: true,
production: false,
Expand Down Expand Up @@ -319,6 +317,14 @@ async function processModule(filePath, cachedIds, registry, ids, envs) {
}
}

if (!isJson) {
const transformed = await esbuild.transform(code, {
format: 'cjs',
target: 'es2015'
});
code = transformed.code;
}

Copy link
Author

@reubenson reubenson Sep 18, 2025

Choose a reason for hiding this comment

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

not sure if this is where this should go, but was getting Unexpected "export" errors

Copy link
Member

@ealcantara22 ealcantara22 Sep 23, 2025

Choose a reason for hiding this comment

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

hi, sorry about the delay. Yes, this is a known issue. I don't know how Browserify and Vuetify detect and resolve these cases. The issue happens when you require a .js service which content is exported using ESM from a .vue file.

// my-service.js
export { ... }
// my-vue-component.vue
const myService = require('../services/my-service');

I gave up on this one back then because we only had to change like 3 or 4 JS files.
Changing export to module.exports should fix it, but let me know if you would rather find the root cause of this issue.

Copy link
Member

@ealcantara22 ealcantara22 Sep 23, 2025

Choose a reason for hiding this comment

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

maybe we can do some replacement to .js files similar to what we do with .vue files here

Copy link
Author

@reubenson reubenson Sep 24, 2025

Choose a reason for hiding this comment

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

I'm good with either solution (can make the change to module.exports), but I think we may as well have the explicit cjs transform. I'm also cool with not making this change for now and moving it to backlog if it comes up again later

// magic-string for file content manipulation
const s = new MagicString(code);

Expand Down