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

[BUGFIX release] ensure import paths are resolved \w posix separators #4205

Merged
merged 1 commit into from
Mar 5, 2016

Conversation

stefanpenner
Copy link
Member

  • share the same algorithm ember.js uses
  • path.join defaults to path[platform].join which results in path.win32.join on windows and path.posix.join on everything else.

@stefanpenner
Copy link
Member Author

still seems to have some issues on windows: ember-cli/ember-cli#5542 (comment)

@stefanpenner
Copy link
Member Author

i think babel may provide a mix of / and \ causing us some grief. amd-name-resolver may want to ensure posix first before processing. Maybe with https://github.com/stefanpenner/ensure-posix-path

@stefanpenner
Copy link
Member Author

tested locally, once ember-cli/amd-name-resolver#4 lands we should be good to go.

@rwjblue
Copy link
Member

rwjblue commented Mar 3, 2016

Ok, looks like it was released and bumped here.

This is good to go...

@floqqi
Copy link

floqqi commented Mar 3, 2016

Correct me if I'm wrong, but shouldn't emberjs/data#fix-windows work now?

C:\Users\floqqi\project> npm install                                                                                                             
ember-data@2.6.0-canary node_modules\ember-data                                                                                
├── ember-cli-path-utils@1.0.0                                                                                                 
├── ember-cli-string-utils@1.0.0                                                                                               
├── ember-cli-test-info@1.0.0                                                                                                  
├── exists-sync@0.0.3                                                                                                          
├── inflection@1.8.0                                                                                                           
├── npm-git-info@1.0.2                                                                                                         
├── semver@5.1.0                                                                                                               
├── babel-plugin-filter-imports@0.2.0                                                                                          
├── amd-name-resolver@0.0.5 (ensure-posix-path@1.0.1)                                                                          
├── chalk@1.1.1 (escape-string-regexp@1.0.5, supports-color@2.0.0, has-ansi@2.0.0, strip-ansi@3.0.1, ansi-styles@2.2.0)        
├── babel-plugin-feature-flags@0.2.0                                                                                           
├── silent-error@1.0.0 (debug@2.2.0)                                                                                           
├── ember-cli-version-checker@1.1.6 (semver@4.3.6)                                                                             
├── git-repo-info@1.1.2                                                                                                        
├── broccoli-file-creator@1.1.0 (symlink-or-copy@1.0.1, rsvp@3.0.21, mkdirp@0.5.1, broccoli-writer@0.1.1, broccoli-kitchen-sink
-helpers@0.2.9, broccoli-plugin@1.2.1)                                                                                         
├── broccoli-merge-trees@1.1.1 (fs-tree-diff@0.4.4, symlink-or-copy@1.0.1, debug@2.2.0, fast-ordered-set@1.0.2, can-symlink@1.0
.0, rimraf@2.5.2, broccoli-plugin@1.2.1)                                                                                       
└── broccoli-babel-transpiler@5.5.0 (clone@0.2.0, json-stable-stringify@1.0.1, broccoli-funnel@1.0.1, broccoli-persistent-filte
r@1.1.8, babel-core@5.8.35)                                                                                                    

Browser throws Uncaught Error: Could not find module '-private/system/references/record' imported from 'ember-data/-private/system/references' anyway.

@kris-ellery
Copy link

@floqqi I can confirm that. It's still throwing an error. I think this is because Ember CLI is not yet released with "amd-name-resolver": "0.0.5". It's on master and in queue for 2.4.2. @stefanpenner is that correct?

@stefanpenner
Copy link
Member Author

@KrisOlszewski ya I believe I updated both sides locally.

@floqqi
Copy link

floqqi commented Mar 4, 2016

@KrisOlszewski Sadly fails anyway with emberjs/data#fix-windowsand ember-cli/ember-cli#6021c6f24d2d31b3fae0ccc8f2a2d8e49fc4cb8e (current master).

@stefanpenner
Copy link
Member Author

@floqqi please provide more info. With the two patches my Windows machine appears happy, so I will need more info

@floqqi
Copy link

floqqi commented Mar 4, 2016

@stefanpenner

OS: Windows 7 x64 SP1
Node: 4.2.1 (npm 2.13.1)
Ember: 2.4.1
Ember-data: emberjs/data#fix-windows
Ember-cli: ember-cli/ember-cli#master

Do you need additional info?

@stefanpenner
Copy link
Member Author

whats the result of ember version in the project?

@floqqi
Copy link

floqqi commented Mar 4, 2016

version: 2.4.1
node: 4.3.2
os: win32 x64

package.json of ember-cli and ember-data contain both "amd-name-resolver": "0.0.5". Also did a fresh install of anything (even node) and the issue still appears.

@stefanpenner
Copy link
Member Author

I dont think you are using ember-cli master. version would be something like: 2.4.1-<some-git-sha>

@floqqi
Copy link

floqqi commented Mar 4, 2016

Hm, but why is than amd-name-resolver at version 0.0.5? Doesn't make much sense to me. Sadly I can't check it out, project is at work and I'm at home now (no Windows available). May @KrisOlszewski has the same issue?

@kris-ellery
Copy link

@stefanpenner and @floqqi unfortunately I have to confirm.

"ember-cli": "ember-cli/ember-cli#master",
"ember-data": "emberjs/data#fix-windows",

amd-name-resolver@0.0.5 (ensure-posix-path@1.0.1) shows in both dependencies and still getting same error in console log as previously.

@stefanpenner
Copy link
Member Author

ill see whats im when im home. I must have fixed something locally and not updated.

@john-griffin
Copy link

Just hit this too, have a project ready to test any fixes that go out.

@bmac
Copy link
Member

bmac commented Mar 5, 2016

@stefanpenner Would you mind squashing this pr down to 1 commit to make it easier for cherry-picking?

* share the same algorithm ember.js uses
* path.join defaults to path[platform].join which results in path.win32.join on windows and path.posix.join on everything else.
bmac added a commit that referenced this pull request Mar 5, 2016
[BUGFIX release] ensure import paths are resolved \w posix separators
@bmac bmac merged commit bc446a1 into master Mar 5, 2016
@bmac
Copy link
Member

bmac commented Mar 5, 2016

I merged this since I believe it solves at least some of the issues on windows. Its sounds like their may be another related issue. I'll try to investigate on a windows box tomorrow.

@bmac bmac deleted the fix-windows branch March 5, 2016 23:40
@john-griffin
Copy link

The module issue is still present when running against master.

@floqqi
Copy link

floqqi commented Mar 7, 2016

Can confirm that the issue is still present using:

"ember-cli": "2.4.2",
"ember-data": "emberjs/data#master"

@floqqi
Copy link

floqqi commented Mar 7, 2016

Is there any plan to address the "related issue"? It's hard to downgrade ember-data since version 2.4.0 broke queryRecord, where promise now gets rejected when no record is found (in version ~2.3.0 it resolves with null).

In the mean time I can't make any production release.

@pangratz
Copy link
Member

pangratz commented Mar 7, 2016

It's hard to downgrade ember-data since version 2.4.0 broke queryRecord, where promise now gets rejected when no record is found (in version ~2.3.0 it resolves with null).

@floqqi This seems like a regression. Can you create an issue for that, ideally with a reproduction using https://ember-twiddle.com/? You can use this as a starting point.

@floqqi
Copy link

floqqi commented Mar 7, 2016

@pangratz Of course, I'll do this later this day.

@nfc036
Copy link

nfc036 commented Mar 7, 2016

On Windows, for a simple project, I get a white page and

Uncaught Error: Could not find moduleember-data/-private\system\references\recordimported fromember-data/-private/system/references``

when using ember-cli@2.4.1 with ember-data@2.4.0 after production build. Same project with ember-cli@2.4.2 creates a production build that is working as expected. Ember is 2.4.1.
I did delete node_modules and a fresh npm install for both tests. Only difference was "2.4.2" instead of "2.4.1" for ember-cli in package.json. HTH.

@floqqi
Copy link

floqqi commented Mar 7, 2016

Now it's getting interesting. @nfc036's tip using

"ember-cli": "2.4.2",
"ember-data": "2.4.0"

is working -> valid production build.

Switching back to

"ember-cli": "2.4.2",
"ember-data": "emberjs/data#master"

causes the issue again (fresh install).

@nfc036
Copy link

nfc036 commented Mar 7, 2016

Same here, using "emberjs/data#master" instead of "2.4.0" produces a production build with console error.

@rwjblue
Copy link
Member

rwjblue commented Mar 7, 2016

Can one of you open a new issue for this? I realize it is related, but I don't want to loose track with this thread being on a merged PR...

@pangratz
Copy link
Member

pangratz commented Mar 7, 2016

@floqqi does #4219 track the regression you were mentioning?

@floqqi
Copy link

floqqi commented Mar 7, 2016

@pangratz yes! I'm gonna provide some more information in the issue.

@pangratz
Copy link
Member

pangratz commented Mar 7, 2016

@floqqi awesome, thanks!

@pwfisher
Copy link

Broken in production build on Linux with

  • ember-cli@2.4.2
  • ember-data@2.4.1
  • amd-name-resolver@0.0.5

Not showing the backslash issue.

Uncaught Error: Could not find module `-private/system/references/record` imported from `ember-data/-private/system/references`

@marcosgz
Copy link

@pwfisher Same thing here. Just updated ember-data from ember-data@2.4.1 to ember-data@2.4.0 and it's working now

@akshayrawat
Copy link

I can confirm what @pwfisher said about this exception exists on Linux production builds as well. Not sure if a fix has been released yet or if there is another issue for this error. I'm currently using @marcosgz workaround which is working well.

@kfuzaylov
Copy link

yes i does not work with ember-data@2.4.1 and works with ember-data@2.4.0

@stefanpenner
Copy link
Member Author

@pwfisher @marcosgz @asakusuma @kfuzaylov you are describing a different issue

@akshayrawat
Copy link

@stefanpenner Is there a Github issue already for it, or should we create a new one?

@stefanpenner
Copy link
Member Author

unknown, but i have opened a PR #4248

@bmac
Copy link
Member

bmac commented Mar 20, 2016

I have released Ember Data 2.4.2 with @stefanpenner's fix for this issue.

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.