Skip to content

preserve-symlinks: default to false - #28

Merged
mightyaleksey merged 1 commit into
css-modules:masterfrom
carlhopf:preserve-symlinks
Sep 5, 2017
Merged

mightyaleksey merged 1 commit into
css-modules:masterfrom
carlhopf:preserve-symlinks

Conversation

@carlhopf

@carlhopf carlhopf commented Sep 1, 2017 •

Copy link
Copy Markdown
Contributor

generate consistent paths for imported files, defaults to nodejs behaviour.

consistency important: f.ex. postcss-icss-selectors :local hashes are based on filenames. browserify defaults to false (like nodejs), hence resolved filenames would be different between browserify plugins and postcss-modules-resolve-imports.

more info: browserify/browserify#1742 nodejs/node#3402

Comment thread lib/resolveModule.js
});

const PRESERVE_SYMLINKS = !!argv['preserve-symlinks'] ||
process.env.NODE_PRESERVE_SYMLINKS + '' === '1';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I usually vote for explicit coercions, cause it's easier to figure out what was the initial intention. So would like to suggest to use here smthing like this:

const PRESERVE_SYMLINKS = Boolean(argv['preserve-symlinks']) ||
  String(process.env.NODE_PRESERVE_SYMLINKS) === '1';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And looks like that minimist has already done necessary typecasting here :)

Comment thread lib/resolveModule.js

function resolveModule(filepath, {cwd, resolve: resolvecfg = {}}) {
const preserveSymlinks = resolvecfg.preserveSymlinks !== undefined
? !!resolvecfg.preserveSymlinks : PRESERVE_SYMLINKS;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just thought that it would be nice to move all the necessary coercions of options (and checking) to the plugin initialisation, so we can keep it at one place. But it doesn't look as an issue at the current step :)

@mightyaleksey
mightyaleksey merged commit 4f958e2 into css-modules:master Sep 5, 2017
@carlhopf
carlhopf deleted the preserve-symlinks branch September 6, 2017 08:13
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.

2 participants