eik-lib / issues

Issues and discussions that span all of Eik
https://eik.dev
1 stars 0 forks source link

RFC 2 - Change globbing in "files" definition to preserve directory structure #2

Closed digitalsadhu closed 2 years ago

digitalsadhu commented 3 years ago

Currently, when using globbing in package.json or eik.json configuration, all matched files are copying into a flat hierarchy before upload to the Eik server.

For example:

Given the following files

/foo/bar.js
/foo/bar/baz.js
/foo.js

And the following files glob definition:

{
  "files": {
    "/": "**/*.js"
  }
}

The final produced uploaded archive will look like this:

foo.js
bar.js
baz.js

Some systems such as Next.js expect their asset files to be in a specific structure.

This RFC therefore proposes 2 solutions.

  1. Non breaking change: Add a flag to allow the preservation of the original folder structure such as --preserveFolders
  2. Breaking change: Change the default to preserve folders and add a flag for the current default behaviour such as --flattenFolders.

Secondary question: Should any flag that is added (whether it be --preserveFolders or --flattenFolders) also be added as a field in config? (eik.json or package.json)

trygve-lie commented 3 years ago

The current flattening; does that apply to all type of packages?

digitalsadhu commented 3 years ago

currently to anything published under the pkg or npm namespaces (not map). Once the changes proposed in #1 are completed it will apply to all.

gingermusketeer commented 3 years ago

Does it need to be a flag or could it only be a config file option? Means there is less to be passed around if it is only in the config.

Also wondering if this could be handled in a non breaking way. For example allowing an array to be specified in the configuration file:

New behaviour

files: [
  {src: "**/*.js", dest: " "/", preserveFolders: true },
]

Old behaviour

files: [
  {src: "**/*.js", dest: " "/"},
]
files: {
  "/": "**/*.js"
}
stipsan commented 3 years ago

IMO it makes sense to have the preserveFolders flag be true by default. If one desires to "flatten" a structure it makes sense that you do that when you export to the folder eik is globbing. Rewriting paths is always a hazard and so I would never expect that to be the default behavior, for the same reasons I don't expect mangle options in uglify/terser to be enabled by default 🤔

alexanbj commented 3 years ago

I actually thought that the current globbing functionality did in fact preserve the folders.

I guess that means I think it should be the default option 🤔

trygve-lie commented 3 years ago

I honestly was not really aware that we flatten the structure either. I am thinking the same as @alexanbj and think we should not do so. I even think we should not offer a feature for flattening at all.

My concern with flattening are:

I would though like to be able to take two (or multiple) source paths and wrap them up into a package.

Lets say one have the following:

/src/
/src/js/
/src/js/main.js
/src/js/components/
/src/css/
/src/css/main.css
/src/test/
/bin/

then with a config do something like this:

files: {
  "/css": "/src/css/**/*"
  "/js": "/src/js/**/*"
}

and one end up with the following structure and files on the server:

/js/
/js/main.js
/js/components/
/css/
/css/main.css
digitalsadhu commented 3 years ago

DECISION

Works for me. I'm beginning to suspect the flat structure may have been accidental based on almost a side effect of how globbing works.

The decision then is that flattening will be changed, directory structures will be preserved and no flags will be added and flattening will not be supported.

The work that needs to be done:

digitalsadhu commented 3 years ago

Where should the root of the folder structure be placed?

To illustrate, lets say this is the folder structure:

/my-project/eik.json
/my-project/dist/
/my-project/dist/nested
/my-project/dist/nested/esm.js
/my-project/dist/styles.css
/my-project/dist/super/nested/file.js

and eik.json is defined with a files block like so:

{
  files: {
    '/file.js': './dist/super/nested/file.js',
    '/': '**/*'
  }
}

We don't know what the users intention is about what constitutes a base directory for the assets. (It's like dist is this case but do they want dist in the URL or not?)

The result we end up with is a set of URLs like so:

https://assets.finn.no/pkg/my-project/1.0.0/eik.json
https://assets.finn.no/pkg/my-project/1.0.0/file.js
https://assets.finn.no/pkg/my-project/1.0.0/dist/styles.css
https://assets.finn.no/pkg/my-project/1.0.0/dist/nested/esm.js

There would be no way for the user to remove the dist part from the url aside from placing the eik.json file in the same folder.

Here are some options:

  1. Figure out the common root of all assets and trim off everything before it. This would work but may produce unexpected results for the user?

  2. Add a configuration option for the asset root, perhaps as a field to eik.json

  3. Do nothing. Perhaps its not the end of the world that the user can remove dist from the URL? If they are really serious about it, they could place their eik.json file in the folder with the assets.

  4. Try to strip up to the glob. Eg. for src/js/main.js, defined as "/": "/src/js/**/*" becomes just https://assets.finn.no/pkg/my-project/1.0.0/main.js

Note also, in the examples above, when mapping a file directly (no globbing) only the file is currently preserved (not the path to where it lies on the fs) which is quite different from globbing. This could also be quite jarring for the user.

Ie. /my-project/dist/super/nested/file.js becomes just https://assets.finn.no/pkg/my-project/1.0.0/file.js and the /dist/super/nested part is trimmed off due to the rule '/file.js': './dist/super/nested/file.js',

Thoughts?

trygve-lie commented 3 years ago

I was thinking about this the other day and I think there is a 5th solution we should consider too.

In most cases I think one only would like to pack up just one folder. Then there are scenarios where one would like to wrap up multiple sources into one package, but that is the exceptions.

In the scenario of just wanting to pack up a folder this is a bit akward syntax:

{
  files: {
    '/': '/my-project/dist/**/*'
  }
}

What if we make file just take a glob string and when its given just a glob string it packages that in the structure it is. Iow:

{
  files: '/my-project/dist/**/*'
}

will give a package containing this (based on the structure given above):

https://assets.finn.no/pkg/my-project/1.0.0/eik.json
https://assets.finn.no/pkg/my-project/1.0.0/nested/esm.js
https://assets.finn.no/pkg/my-project/1.0.0/styles.css
https://assets.finn.no/pkg/my-project/1.0.0/super/nested/file.js

Then we can also let file take an object literal where the keys must be a name which can be a relative path excluding /. Iow:

{
  files: {
    a: '/my-project/dist/**/*',
    b: '/my-project/out/js/some_file.js',
  }
}

will give a package containing this (based on the structure given above):

https://assets.finn.no/pkg/my-project/1.0.0/eik.json
https://assets.finn.no/pkg/my-project/1.0.0/a/nested/esm.js
https://assets.finn.no/pkg/my-project/1.0.0/a/styles.css
https://assets.finn.no/pkg/my-project/1.0.0/a/super/nested/file.js
https://assets.finn.no/pkg/my-project/1.0.0/b/some_file.js

This is then not a legal syntax:

{
  files: {
    a: '/my-project/dist/**/*',
    '/': '/my-project/out/js/**/*',
  }
}
digitalsadhu commented 3 years ago

Ok, so what are the rules there? Something like:

Files is a string

Files is an object; prefix with key

This seems good to me. Can't see any issues but have 2 questions:

  1. This is very breaking and we have a lot of usages out in the world already, should we support / and ./ for backwards compatibility but deprecate them?
  2. Is there any value is supporting folder as well as in non glob, non file values such as files: { a: '/my-project/dist' } or files: '/my-project/dist'. This would be identical to /my-project/dist/**/* but without the glob syntax at the end.
trygve-lie commented 3 years ago

Ok, so what are the rules there?

Yes. You describe the rules in the same way I'm thinking.

This is very breaking and we have a lot of usages out in the world already, should we support / and ./ for backwards compatibility but deprecate them?

I am fine with breaking at this stage. Usage is not that large.

Is there any value is supporting folder as well as in non glob, non file values such as files: { a: '/my-project/dist' } or files: '/my-project/dist'. This would be identical to /my-project/dist/*/ but without the glob syntax at the end.

If its easy to support, that would be great.

digitalsadhu commented 3 years ago

Some examples of values that can be given to files in eik.json (or package.json)

// everything in the `<cwd>/folder` is uploaded to /
files: 'folder'

// everything in the `<cwd>/folder` is uploaded to /
files: './folder'

// everything in the `<cwd>/folder` is uploaded to /
files: './folder/'

// everything in the `<cwd>/folder/nested` is uploaded to /
files: './folder/nested'

// everything in the `<cwd>/folder` is uploaded to / (no directory recursion)
files: './folder/*'

// everything in the `<cwd>` is uploaded to / (dont do this!)
files: './**/*'

// everything in the `<cwd>/folder` is uploaded to /
files: './folder/**/*'

// everything in the `/path/to/folder` is uploaded to /
files: '/path/to/folder/**/*'

// everything in the `/path/to/folder` is uploaded to /
files: '/path/to/folder'

files: {
    // file `<cwd>/path/to/esm.js` is uploaded and renamed to `/script.js`
    'script.js': './path/to/esm.js',

    // file `/absolute/path/to/esm.js` is uploaded and renamed to `/script.js` 
    'script.js': '/absolute/path/to/esm.js',

    // everything inside `/absolute/path/to/folder` is uploaded to `/folder`
    'folder': '/absolute/path/to/folder',

    // everything in `<cwd>/path/to/folder` is uploaded to `/folder`
    'folder': './path/to/folder',

    // everything in `<cwd>/path/to/folder` is uploaded to `/folder`
    'folder': 'path/to/folder',

    // everything in `/absolute/path/to/folder` is uploaded to `/folder`
    'folder': '/absolute/path/to/folder/**/*',

    // everything in `<cwd>/path/to/folder` is uploaded to `/folder` (but no folder recursion)
    'folder': './path/to/folder/*',

    // everything in `<cwd>/path/to/folder` is uploaded to `/folder`
    'folder': 'path/to/folder/**/*',

    // everything in `<cwd>/path/to/folder` is uploaded to `/folder/scripts`
    'folder/scripts': 'path/to/folder/**/*',
}
trygve-lie commented 3 years ago

That looks about right