initial psc-package support #82
Conversation
|
Thank you for the PR! I will take a look and review this soon. |
README.md
Outdated
|
|
||
|
|
||
|
|
||
| Set `pscPackage` query parameter to `true` and delete `src` parameter to enable `psc-package` support |
There was a problem hiding this comment.
Can we say remove or unset instead of delete here?
src/index.js
Outdated
| @@ -10,6 +10,8 @@ const PsModuleMap = require('./PsModuleMap'); | |||
| const Psc = require('./Psc'); | |||
| const PscIde = require('./PscIde'); | |||
| const dargs = require('./dargs'); | |||
| const execSync = require('child_process').execSync | |||
There was a problem hiding this comment.
Just to ensure compatibility can we change this to cross-spawn? We run psc, etc., already with cross-spawn.
There was a problem hiding this comment.
I did notice you're using cross-spawn, and did honest try to run psc-package asynchronously, but ran into weird synchronization issue and decided to fall back to execSync. What I didn't try is to use cross-spawn's sync. On it.
There was a problem hiding this comment.
All right, my bad: I've forgot to remove src query parameter in webpack.config.js, that's why source paths have been ignore. I probably should add a warning about pscPackage being ignored if src is set.
So looks like there were no synchronization issues, just me.
src/index.js
Outdated
| bundleOutput: 'output/bundle.js', | ||
| bundleNamespace: 'PS', | ||
| bundle: false, | ||
| warnings: true, | ||
| output: 'output', | ||
| src: [ | ||
| path.join('src', '**', '*.purs'), | ||
| path.join('bower_components', 'purescript-*', 'src', '**', '*.purs') | ||
| ...depsPaths(query.pscPackage) |
There was a problem hiding this comment.
Does npm run build work with the spread operator? We just have es2015 as the preset in the .babelrc. Do we need a babel plugin to support this?
There was a problem hiding this comment.
It totally did work for me, will look into it.
There was a problem hiding this comment.
All right, es2015 preset does include spread operator so the answer is no, we don't.
src/index.js
Outdated
| bundleModules: [], | ||
| warnings: [], | ||
| errors: [] | ||
| } |
There was a problem hiding this comment.
Sorry to be picky, but can we put the spacing back to what it was on lines 61 to 66. It looks like just an extra space (or two) has been added.
There was a problem hiding this comment.
Yeah, that's IDEA being smartass, reverting. Is there some way to specify indents explicitly? .editorconfig maybe?
|
Looking good. Just a few small comments. You mention it sort of works with the example. Are you still in-progress on this PR? Just curious. Thanks again! |
|
Thank you for the feedback!
Yes, this way:
That depends of whether I need to implement adding |
|
And oh, there's no check for |
|
OK, here's my attempt to use const spawn = require('cross-spawn').sync
//...skip...
const depsPaths = (pscPackage => {
if (pscPackage) {
debug('calling psc-package...')
const paths = spawn('psc-package', ['sources']).stdout.toString().split(eol).filter(v => v != '');
debug(paths)
return paths
}
else {
return [path.join('bower_components', 'purescript-*', 'src', '**', '*.purs')];
}
})Here's the output:Note the source paths have been retrieved but weren't passed to |
develop7
force-pushed
the
feature/psc-package
branch
from
30d7b86 to
081c17d
Compare
|
@ethul all right, seems to be ready to roll. I've added a warning that To be compatible with user-supplied paths and be less intrusive, I probably should extract Thank you in advance. |
|
Thanks so much for working on this and making all of the updates! This is looking great. I will go through it over the weekend. Thanks again. |
|
I think the changes look good. However, after thinking about this more, I wonder if the psc-package functionality should append the results of running Consider the following cases:
src: [
path.join('src', '**', '*.purs'),
path.join('bower_components', 'purescript-*', 'src', '**', '*.purs')
]
src: [
path.join('src', '**', '*.purs'),
...results of running psc-package sources
]
src: [
...user specified src,
...results of running psc-package sources
]What do you think? I am leaning toward the behaviour defined by the three cases listed above because I think it satisfies if the user wants to keep using the existing The way that we have it set up in the PR at the moment is that the user would be unable to take advantage of If we decide to go the route described in the three cases above then I don't think we need the warning about specifying Finally, and this is small request, but would you be willing to pull out lines 24-33 into a separate source file called |
Totally agree. Done in e92e5f3 :)
Sure, I'm just not sure the file should be called this way — line 31 specifically mentions |
Also reflect pscPackage support in "Default options" section
|
Thank you very much for making the updates! Looks good. Also, no worries about extracting to a separate file. This can be done later on if we determine it is necessary. |
Initial and very basic support of
psc-package. Sort of works withpurescript-webpack-example(deps' paths are supplied to compiler, but compilation still fails)This change is