PrismJS / prism-themes

A wider selection of Prism themes
MIT License
1.41k stars 502 forks source link

build: add Node.js main file #116

Closed Kikobeats closed 3 years ago

Kikobeats commented 3 years ago

Hello,

In a previous PR (#114) the main field at package.json was removed because it was causing not useful.

This PR adds the field again but rather than pointing to a filesystem folder, it's returning the absolute path where the themes are living:

require('prism-themes')
// {
//  folderPath: '/Users/kikobeats/Projects/microlink/browserless/packages/screenshot/node_modules/prism-themes/themes'
// }

This is actually useful because for some bundle workflow you need the reference of the folder to copy or load the themes.

In this way, it prevents copy the whole folder, but you still have a reference to the folder path in case you need it.

RunDevelopment commented 3 years ago

Why do we need to export this path? Couldn't somebody that needs the path just do require.resolve('prism-themes/themes')? It's not like we plan to rename the themes folder anytime soon (since that would be a breaking change).

Kikobeats commented 3 years ago

@RunDevelopment that was my initial assumption and it's failing:

https://github.com/microlinkhq/browserless/blob/master/packages/screenshot/src/pretty/theme.js#L9

The assumption is wrong since the folder is going to live depending on where npm install decides to place the folder, meaning that if the project has other dependencies using prism-themes it will be placed in a place where both dependencies can find it, making the path unpredictable.

The solution is to use "main" on package.json to point to the dependency path, and because the dependency path is calculated using __dirname, you always will have the absolute path!

RunDevelopment commented 3 years ago

@Kikobeats Not path.resolve but require.resolve.

Kikobeats commented 3 years ago

@RunDevelopment require.resolve can't find the module since the package doesn't have the "main" field

Screen Shot 2021-03-23 at 15 18 23
RunDevelopment commented 3 years ago

require wants to find .js files, right... my bad. A main module is necessary in that case.

Kikobeats commented 3 years ago

fixed lint issues!

Kikobeats commented 3 years ago

added missing semicolon

Kikobeats commented 3 years ago

I understand although literally I just added 5 lines of javascript to make possible use it like the rest of npm dependencies

Kikobeats commented 3 years ago

@RunDevelopment any status of this? really want to use the library 😭

RunDevelopment commented 3 years ago

Sorry for the delay @Kikobeats. Let's merge this.

Kikobeats commented 3 years ago

@RunDevelopment thanks a lot! can you please release a new version?

RunDevelopment commented 3 years ago

Just published a new version @Kikobeats.