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

Fix using colon in the filename on Windows #506

Closed
wants to merge 4 commits into from

Conversation

aminya
Copy link

@aminya aminya commented Jul 23, 2020

Rollup Plugin Name: multi-entry

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes: I included the real-world tests.

Previously the tests were using getCode which is not what happens in the real world. This fixes an already broken plugin on Windows. To see the difference, you need proper Windows machine or Windows support in Repl.it to run the tests before and after.

  • no:

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
Fixes #488

Description

use U+A789 instead of colon in the file name on Windows

@aminya aminya force-pushed the windows_multientry branch 3 times, most recently from 4bacea4 to 88efc37 Compare July 24, 2020 00:06
@aminya aminya marked this pull request as draft July 24, 2020 00:10
@aminya aminya changed the title use U+A789 instead of colon in the file name on Windows Fix using colon in the filename on Windows Jul 24, 2020
@aminya aminya marked this pull request as ready for review July 24, 2020 00:12
@frank-dspeed
Copy link
Contributor

@shellscape @lukastaegert can you please review this it is the fix for the windows : char not supported issue of plugin multi entry

#488 (comment)

@shellscape
Copy link
Collaborator

Patience, please. This PR is less than 24 hours old.

let entry;
if (process.platform === "win32") {
// the colon is different
entry = '\0rollup꞉plugin-multi-entry꞉entry-point';
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is an interesting hack (great thinking). @lukastaegert can you see a situation in which this would cause problems? if not, we should just unify on that unicode character for all platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shellscape i am not sure how linux handels that unicode char at last inside bash i could expect that copy paste of that file name will result into not opening the file but it needs testing i never used such chars on linux filesystems before i think it is clever to do this only for windows so that nothing else breaks that already works.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shellscape i see a problem with the process.platform variable used inside browser should be no global process at last.But in general a if condition is the way to go so we break not as much.

Copy link
Author

Choose a reason for hiding this comment

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

The name of the final bundle is now _rollup꞉plugin-multi-entry꞉entry-point.js. Is this the desired behavior?

My question is why you don't give an option to the user for choosing the name?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, really stupid question, but there is no reason at all to have those colons, and using a clever character substitution that will even depend on the system running it does not feel like a good idea. So why not just a new name? And while we are at it considering that this is becomes a real file name when using the dir, why not just entry.js or index.js? I also think that there is no reason at all to use the \0 prefix, on the contrary, it could be beneficial if a user could run Babel transforms on that file. And in the long run, it might make sense to make the actual name a config option of this plugin for the case when the dir option is used.

Copy link
Member

Choose a reason for hiding this comment

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

We would also need a test for that case and the new option, as well as a test using the dir option without specifying the entry name.

Copy link
Author

@aminya aminya Jul 28, 2020

Choose a reason for hiding this comment

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

This is just a patch. I'd say let's merge this as it is to fix the issue temporarily. In a separate pull request, we can make a feat to add options. However, we should not put this patch on hold for that.

Choose a reason for hiding this comment

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

@aminya I suggest using another clean name like rollup_plugin_multi_entry_entry_point

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be my requirement as well. If you think that would be breaking, then your patch would be breaking as well as it changes the name on Windows. But worse, it will make the windows build differ from what is generated on other systems.
In any case, even if the option is not added, a single test using the dir option would still be important.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aminya I suggest using another clean name like rollup_plugin_multi_entry_entry_point

Please no. That's damn ugly.

@frank-dspeed
Copy link
Contributor

i have looked into this and i will make the output filename configure able i have looked what it does and it was made befor the dir option existed.

@cyclops24
Copy link

cyclops24 commented Jul 28, 2020

@aminya @lukastaegert @frank-dspeed Guys, I didn't understand Why do we want to complicate matters? I think just a simple file name change from \0rollup:plugin-multi-entry:entry-point to rollup_plugin_multi_entry_entry_point is enough. Why we need a parameter for configuring that or using a special character like U+A789 to keep this messy : in the file name. Why we need this : or parameter? 🤔

also make this configurable is another problem and it's a new feature why we need mixing that feature request with this issue?
We now just need a fix PR to change \0rollup:plugin-multi-entry:entry-point to rollup_plugin_multi_entry_entry_point. I can create once if you want.

@frank-dspeed
Copy link
Contributor

frank-dspeed commented Jul 28, 2020

@cyclops24 ok i will explain it to you

the plugin generates a virtual entrypoint a file that does not exist and imports the other files inside that

then it wants to write that file but it has no name so this virtual ...... name gets used the option is needed to supply a usefull filename for the output set able by the user.for dynamic imports the normal chunkFileName Patterns get used.

I hope you understand that now more well.

at present plugin-virtual is a drop in replacement for this when u use glob to set the content of the virtual module.

@cyclops24
Copy link

cyclops24 commented Jul 28, 2020

@frank-dspeed Thanks for your description. 🌹 It was as I imagined.
Just one question:
So the : character in the filename is important for this plugin-virtual?
Have we any convention about this?
You mean if we change \0rollup:plugin-multi-entry:entry-point to rollup_plugin_multi_entry_entry_point for fix this issue then we breaks plugin-virtual or other things?

My suggestion is clear:
Change \0rollup:plugin-multi-entry:entry-point to rollup_plugin_multi_entry_entry_point as a fix for this issue and close #488 and then make new PR to add a parameter to customize this file name.

@frank-dspeed
Copy link
Contributor

@cyclops24 you can change it and your done simply copy the content of the plugin into your config and change that to what ever filename you want it will work. but rollup will need to solve it via a option.

@shellscape
Copy link
Collaborator

Change \0rollup:plugin-multi-entry:entry-point to rollup_plugin_multi_entry_entry_point as a fix for this issue and close #488 and then make new PR to add a parameter to customize this file name.

I will block this suggestion. If we're going to fix this, we fix it right. I'm not going to approve a half measure just to close an issue.

Copy link
Collaborator

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

You should have the tests already. This fixes an already broken plugin on Windows. To see the difference, you need proper Windows CI or a Windows support in Repl.it. If you had the Windows tests in the first place, this would have not happened.

No, this is not correct. If you examine the checks you'll see that we have two workflows through Github Actions that run on windows. This PR will not be merged until the proper tests are added. You can use os.platform to add platform-specific tests.

@cyclops24
Copy link

cyclops24 commented Jul 29, 2020

Change \0rollup:plugin-multi-entry:entry-point to rollup_plugin_multi_entry_entry_point as a fix for this issue and close #488 and then make new PR to add a parameter to customize this file name.

I will block this suggestion. If we're going to fix this, we fix it right. I'm not going to approve a half measure just to close an issue.

As I told before you have combined the fix and the new feature. This is a fix PR. So we don't need to add a new parameter to configure the output file name in this PR.
The output filename parameter is a new feature and needs a separate feature PR.
But anyway if you prefer to keep this known bug and tie this to that new feature, do it. It was just my suggestion 😉

@shellscape
Copy link
Collaborator

shellscape commented Jul 29, 2020

This is a fix PR. So we don't need to add a new parameter to configure the output file name in this PR. The output filename parameter is a new feature and needs a separate feature PR.

We have multiple options in the pull request template that are not mutually exclusive. Our PRs can be one or many of: bugfix, feature, refactor, documentation, or other. Your suggestion is noted and appreciated, but as a maintainer of the repo I have chosen to reject the suggestion as a half-measure.

@aminya aminya requested a review from shellscape July 29, 2020 17:26
@aminya
Copy link
Author

aminya commented Jul 29, 2020

I fixed the tests of multi-entry plugin. This is more ready to get merged.

@aminya aminya requested a review from lukastaegert July 29, 2020 20:38
@frank-dspeed
Copy link
Contributor

frank-dspeed commented Jul 30, 2020

Only to make this complet we can replace this multi entry by @rollup/plugin-virtual

import virtual from '@rollup/plugin-virtual';
import { promise as matched } from 'matched';

let entry = 'filename'
let include = [];
let exclude = [];

/* You can choose if you want to import only or export  */
let prefix = 'export * from' // 'import'
let exporter = (path) => `${prefix} ${JSON.stringify(path)};`;
const patterns = include.concat(exclude.map((pattern) => `!${pattern}`));

const entrys = matched(patterns, { realpath: true }).then((paths) =>
    paths.map(exporter).join('\n')
);

export default {
  input: entry,
  // ...
  plugins: [
    virtual({ [entry]: entrys })
  ]
};

my current version of this plugin

import { promise as matched } from 'matched';

export default function multiEntry(conf={}) {
   const config = { 
    include: [],
    exclude: [],
    exports: true,
    outputFile: 'multi-entry',
    ...conf
  }
  let { outputFile, include, exclude, exports } = config;
  if (include.length === 0) {
      // We need any includes
      throw new Error('We need Include and a outputFile')
  }
  if (typeof include === 'string' || typeof include === 'array'){
      new Error('type of include needs to be string or array of strins')
  }
  
  if (typeof exclude === 'string' || typeof exclude === 'array'){
    new Error('type of exclude needs to be string or array of strins')
  }

  //String option to array
  if (typeof exclude === 'string') {
    exclude = [exclude]
  }   
  if (typeof include === 'string') {
    include = [exclude]
  }   

  let prefix = exports ? 'export * from' : 'import'
  let exporter = (path) => `${prefix} ${JSON.stringify(path)};`;

  return {
    buildStart() {
       this.emitFile({ type: 'chunk', id: outputFile })
    },
    resolveId(id) {
      return id === outputFile ? id : null;
    },
    load(id) {
      if (id === outputFile) {
        const patterns = include.concat(exclude.map((pattern) => `!${pattern}`));
        return matched(patterns, { realpath: true })
            .then(paths => paths.map(exporter).join('\n'));
      }
      return null
    }
  };
}

@Bluefinger
Copy link
Contributor

@frank-dspeed As a note regarding your code example, there's no such thing as 'array' for typeof. Arrays are 'object' type 😄 so you'd want to ensure the checks use Array.isArray. Plus your type guard checks need to be inverted, so typeof include !== 'string'. Anyway, a bit off topic so I'mma just go back to keeping an eye on this PR.

@frank-dspeed
Copy link
Contributor

frank-dspeed commented Jul 30, 2020

@Bluefinger good catches it was only to demo the algo any way so pick up the code that you like and issue a PR that would help the people i am at present working on a diffrent front i want to get ESM interop fixed everywhere so i am kinda busy.

i vote strong for reuse of plugin-virtual it would be a good example and reciepe for others.

It would be clever to create a new clean PR with a good version @Bluefinger

@aminya
Copy link
Author

aminya commented Jul 31, 2020

This is rebased to fix the conflicts caused by #513. This is ready from my side.

Copy link
Collaborator

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

I agree with the reply: https://github.com/rollup/plugins/pull/506/files#r461395228 that this should be made configurable and that an option is the proper fix (even though it is a new feature, technically). I stand by my reply: #506 (comment).

We'll have to wait for this to be made configurable before approving this PR. Please avoid additional discussion about this decision, replies doing so will be hidden as off-topic.

Update: I just ran across #520. It may supersede this PR, but can't say for certain until we get eyes on it.

@aminya aminya closed this Aug 1, 2020
@aminya

This comment has been minimized.

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

Successfully merging this pull request may close these issues.

multi-entry plugin no such file error
6 participants